666446 - lots of animated gifs swamp us with paint events

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

An example of this happening is here:

http://chat.vitebsk.ws/smile.php?name=kolobki2&jsclose=1&PHPSESSID=712fb5f997934bf52dbaf00e73fe81ea

It would be interesting to know if this a regression.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

The obvious solution is to coalesce paint events.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #0)
> An example of this happening is here:
> 
> http://chat.vitebsk.ws/smile.
> php?name=kolobki2&jsclose=1&PHPSESSID=712fb5f997934bf52dbaf00e73fe81ea
> 
> It would be interesting to know if this a regression.

Another side effect of not coalescing shows up on OS X. When Firefox is in the background the system throttles the number of paint events, this causes the animations to run slowly. I don't see the same problem in Chrome.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

We probably have a couple of different ways to solve this problem:

1. Animate GIFs using nsRefreshDriver instead of their own timers. This is probably the ideal solution, but might require a fair amount of work because I believe we'll need to be able to paint a GIF at particular time instead of just the current frame.

2. Trigger paints using nsRefreshDriver but animate the images using individual timers still. We still spin the event loop as often, but we don't try to paint really fast.

3. Coalesce invalidations. The basic idea here is to accumulate invalidation areas until a certain threshold before sending them to the OS. This is general solution to the problem, but I'm not sure it's the direction we want to go.

It would be interesting to know which solution WebKit uses.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #3)
> 2. Trigger paints using nsRefreshDriver but animate the images using
> individual timers still.

This strikes me as the most straightforward.  I'm not sure we could do #1, because nsRefreshDrivers are tab-specific, whereas images aren't.

Comment 5

[Scott Johnson (:jwir3)]

Since I'm running Ubuntu right now, I added some printing code to the widget/src/gtk2/nsWindow.cpp file, in OnExposeEvent, which dholbert advised me might be the platform-dependent version of nsWindowGfx::OnPaint(). I had it print a counter to determine how much it's being called, with it printing every 10 counts. 

This was run on an intel Core i7-2820QM, with 8GB RAM. I found the following:

1) When using Nightly to view several news pages (some images, mostly text), retrieved from igoogle.com main site, over the period of ~1min resulted in about 1280 calls to OnExposeEvent().

2) When using the same version, but viewing the link in the Description, over about the same amount of time (~1min), I observed 7590 calls to OnExposeEvent(). Also, this was without any page reloads or modification of the address bar (aside from the first typing of the address in the bar). 

These are mostly just stats - I'm currently working on analysing this further and looking into a possible fix.

Comment 6

[Scott Johnson (:jwir3)]

Attachment: Example of a situation where the proposed solution might be lacking.

Comment 7

[Scott Johnson (:jwir3)]

Attachment: Timeline of the problematic situation

Comment 8

[Scott Johnson (:jwir3)]

dbaron, dholbert, and I were talking this afternoon. As Daniel suggested in comment 4, we could trigger repaints using nsRefreshDriver instead of the image's Notify() routine. David pointed out a problem where we might have an out-of-sync issue with respect to some painting.

For example, consider the first attachment ( https://bugzilla.mozilla.org/attachment.cgi?id=541840 ). It is an image of two frames, one being an image animation, the other being a partially transparent frame that partially overlays the image animation. 

In the second image ( https://bugzilla.mozilla.org/attachment.cgi?id=541841 ), I've attached a possible timeline for refresh behavior. 'IA' indicates where an image's Notify() routine would normally have been called to invalidate the RasterImage, and (currently) also paint the new image in the window, and 'RD' indicates a location where the nsRefreshDriver would paint the image to the window. If we move the invalidate call into the refresh driver, then we may have a situation where the RasterImage has already been updated, thus (assuming we were previously at frame x) memory now has the frame x+1 of the image already loaded. Since the invalidate call has been delayed until the refresh driver is next triggered, the image painted on the screen is still frame x. If, however, an invalidation happens to part of the image, as could happen if an invalidate call was made to the transparent overlaying area, then we have a situation where the pink area of the image above is redrawn, but because the RasterImage has frame x+1 in it, it is redrawn as x+1, whereas the rest of the image is still at frame x until the next call to the refresh driver.

So, the question is how to overcome this. One suggestion that was in the bug is to animate images using the refresh driver instead of their own timers, thus moving all of the code into the refresh driver for repainting. As mentioned in comment 3 above, though, this could be quite difficult.

If we decide that this might be too much work for the time being, then I could look into implementing the solution mentioned where we utilize an image's own timers to control animation, but do the repainting in the refresh driver, and then try to gauge how much of a problem this will turn out to be.

Other thoughts?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Animating images off the refresh driver sounds good, but one thing that'll make it hard is that the image cache is global and the refresh driver is best per-window, so the same image can be animating in multiple windows and animating it in one window could make it animate "too early" in another window. That's probably why Jeff said "we'll need to be able to paint a GIF at particular time instead of just the current frame". Then again, we have this bug currently where an animated image loaded in two windows is forced to use the same animation timeline, which is wrong.

I'll think about it.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe we could make imgRequestProxy detect animated images and forcefully create a new imgIRequest for each instance of an animated image, with the requests sharing the underlying image data but with separate decoders, so each animated image instance gets its own timeline. Then we could control the animation from the refresh driver.

Comment 11

[Scott Johnson (:jwir3)]

Here's a proposed fix that dholbert and I have been working on. It does require changes to the imgIContainer API.

Current Ownership Model:
            nsDocument
             /       \
nsRefreshDriver    nsImageFrame
                      |
                      |
  (another proxy)  imgRequestProxy   (another proxy)
          \________   |   _________________/
                   \  |  /
                   imgRequest
                      |
                   imgIContainer (RasterImage, VectorImage)
Goal: 

Have imgIContainers register with a RefreshDriver, so they can get rid of their internal timers, and so we don't get a large number of sequential invalidates & repaints on pages with a very large number of animated GIFs
NOTE: This proposed fix will require changes to the imgIContainer API.

SETUP:
At some point, when we know we're an animated image but before animation starts (maybe the first time EvaluateAnimation succeeds -- this is just after our second frame decodes, for raster images), our imgIContainer fires a callback that makes imgRequest do the following:

For each imgRequestProxy:

    Clone the imgRequest & imgIContainer, and call imgRequestProxy::ChangeOwner to make the proxy point to its own personal imgRequest-clone.
      * We need information about which instance of an animation is at which frame, so the clones keep track of this for us. Each clone would have a member variable indicating which frame that instance is currently displaying.

      * Ideally, clones should share decoded image data. Maybe they're "RasterImageWrapper" or some other lightweight friend helper class that implements imgIContainer?

    If that succeeds, the imgIContainer or imgRequest then calls some new method like imgIContainerObserver::ReadyToStartAnimating()

    This makes it to the nsImageFrame (or some similar layout object), which then registers itself (or a helper class) as a refresh observer.

SAMPLING:

    nsRefreshDriver fires a tick, triggering ::WillRefresh(timestamp) on nsImageFrame (or an analogous layout object)

    This calls ::RefreshTick(timestamp) on the imgIContainer. 

    RasterImage::RefreshTick implementation:

      * Examines timeStamp and figures out what the correct frame for that timestamp is.

      * If that's different from the current frame, we call OnFrameChanged. (this is what RasterImage::Notify does now.)

    VectorImage::RefreshTick impl:

      * Seeks internal document to the correct time.

      * That will automatically trigger invalidations if appropriate, via VectorImage::InvalidateObserver()

      * The internal SVG document can now be Pause()d (which prevents ticks from *its* RefreshDriver from affecting animations)

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to comment #11)
> Goal: 
> 
> Have imgIContainers register with a RefreshDriver

Oops, I forgot to update this part -- "register" should say "register (indirectly)" there. (since it's technically the layout object that is the refresh observer, and it passes ticks down to its imgIContainer)

Comment 13

[Joe Drew (not getting mail)]

(In reply to comment #11)
>     Clone the imgRequest & imgIContainer, and call
> imgRequestProxy::ChangeOwner to make the proxy point to its own personal
> imgRequest-clone.

I understand that you want to be able to seek to arbitrary timestamps, but this is really a lot of extra code and complexity for something of a niche requirement.

>     This makes it to the nsImageFrame (or some similar layout object), which
> then registers itself (or a helper class) as a refresh observer.

Do refresh drivers still need to be associated with a docshell? A while ago, Boris mentioned to me that nsRefreshDriver doesn't work with image documents; is this still the case?

(For image documents, my understanding is that we have no layout objects at all.)

>     RasterImage::RefreshTick implementation:
> 
>       * Examines timeStamp and figures out what the correct frame for that
> timestamp is.
> 
>       * If that's different from the current frame, we call OnFrameChanged.
> (this is what RasterImage::Notify does now.)

As mentioned above, I think a better option would be to evaluate animation globally. This should probably be a lot simpler.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #13)
> (In reply to comment #11)
> >     Clone the imgRequest & imgIContainer, and call
> > imgRequestProxy::ChangeOwner to make the proxy point to its own personal
> > imgRequest-clone.
> 
> I understand that you want to be able to seek to arbitrary timestamps, but
> this is really a lot of extra code and complexity for something of a niche
> requirement.

You mean the requirement of having different instances of an animated image have their own timelines?

> >     This makes it to the nsImageFrame (or some similar layout object), which
> > then registers itself (or a helper class) as a refresh observer.
> 
> Do refresh drivers still need to be associated with a docshell? A while ago,
> Boris mentioned to me that nsRefreshDriver doesn't work with image
> documents; is this still the case?

I'm not sure what he meant. An image document belongs to a docshell, but more importantly it has a prescontext/presshell, and I'm pretty sure it has a refresh driver.

> (For image documents, my understanding is that we have no layout objects at
> all.)

This is not true. It builds a normal HTML document containing an <img> element, and has a regular frame tree.

> >     RasterImage::RefreshTick implementation:
> > 
> >       * Examines timeStamp and figures out what the correct frame for that
> > timestamp is.
> > 
> >       * If that's different from the current frame, we call OnFrameChanged.
> > (this is what RasterImage::Notify does now.)
> 
> As mentioned above, I think a better option would be to evaluate animation
> globally. This should probably be a lot simpler.

Can you be more specific? I'm not sure what option you're referring to.

Comment 15

[Daniel Holbert [:dholbert]]

(In reply to comment #14)
> (In reply to comment #13)
> > As mentioned above, I think a better option would be to evaluate animation
> > globally. This should probably be a lot simpler.
> 
> Can you be more specific? I'm not sure what option you're referring to.

I'm guessing that joe's suggesting that we replace the per-RasterImage timers with a single global timer, whose callback synchronously calls RasterImage::Notify on all the animated images in the image-cache.

That would batch invalidations, which could indeed be a simpler & more targeted solution for this bug's specific perf issues...

Comment 16

[Daniel Holbert [:dholbert]]

(In reply to comment #14)
> > I understand that you want to be able to seek to arbitrary timestamps, but
> > this is really a lot of extra code and complexity for something of a niche
> > requirement.
> 
> You mean the requirement of having different instances of an animated image
> have their own timelines?

joe: note also that having per-instance (or per-document) image timelines would have the benefit of fixing things like bug 663800, too.

Comment 17

[Joe Drew (not getting mail)]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #11)
> > >     Clone the imgRequest & imgIContainer, and call
> > > imgRequestProxy::ChangeOwner to make the proxy point to its own personal
> > > imgRequest-clone.
> > 
> > I understand that you want to be able to seek to arbitrary timestamps, but
> > this is really a lot of extra code and complexity for something of a niche
> > requirement.
> 
> You mean the requirement of having different instances of an animated image
> have their own timelines?

Right. (That's what I meant by "global" too.) That requirement seems completely orthogonal to paint events.

I don't think we should work towards different timelines for different instances of images right now, and possibly not at all.  (The simplest and maybe best way to do this would be to duplicate animated images, rather than sharing the image data, imo.)

> I'm not sure what he meant. An image document belongs to a docshell, but
> more importantly it has a prescontext/presshell, and I'm pretty sure it has
> a refresh driver.
> 
> > (For image documents, my understanding is that we have no layout objects at
> > all.)
> 
> This is not true. It builds a normal HTML document containing an <img>
> element, and has a regular frame tree.

Good. I'm glad to be wrong here! :)

Comment 18

[Boris Zbarsky [:bzbarsky]]

> A while ago, Boris mentioned to me that nsRefreshDriver doesn't work with
> image documents; is this still the case?

I have no idea what that's about... I can only assume that either I misspoke or you misunderstood or both....

One issue we've had with refresh driver for animations in the past that hasn't been brought up here is that if an image is supposed to change frames every 250ms then running that off the 60Hz refresh driver timer is pretty wasteful.  Not sure what we can do about it or whether we should worry about it, but that's why Alon hadn't tried to hook up animations to refresh driver in the past.  Another issue is that weird things could happen in background tabs (e.g. we might need to suddenly run though a lot of the animation when switching to that tab.

There's some related discussion in bug 571394, by the way.

Comment 19

[Boris Zbarsky [:bzbarsky]]

And in particular, see the discussion there about image animations and grouping their timers.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #17)
> I don't think we should work towards different timelines for different
> instances of images right now, and possibly not at all.  (The simplest and
> maybe best way to do this would be to duplicate animated images, rather than
> sharing the image data, imo.)

Sharing timelines is a bug, e.g. bug 663800.

A single global timer would fix this particular bug more easily, but it wouldn't fix bug 663800, and wouldn't give us other benefits of refresh driver control such as throttling image animation in hidden tabs.

(In reply to comment #18)
> One issue we've had with refresh driver for animations in the past that
> hasn't been brought up here is that if an image is supposed to change frames
> every 250ms then running that off the 60Hz refresh driver timer is pretty
> wasteful.  Not sure what we can do about it or whether we should worry about
> it, but that's why Alon hadn't tried to hook up animations to refresh driver
> in the past.

I think we could extend nsRefreshDriver::AddRefreshObserver to take a delay parameter, and allow up to that much time to elapse before the refresh driver timer fires.

> Another issue is that weird things could happen in background
> tabs (e.g. we might need to suddenly run though a lot of the animation when
> switching to that tab.

We could add another parameter to AddRefreshObserver (or a new method) to let an observer be called at low frequency (1Hz?) for background tabs.

Comment 21

[Joe Drew (not getting mail)]

(In reply to comment #20)
> (In reply to comment #17)
> > I don't think we should work towards different timelines for different
> > instances of images right now, and possibly not at all.  (The simplest and
> > maybe best way to do this would be to duplicate animated images, rather than
> > sharing the image data, imo.)
> 
> Sharing timelines is a bug, e.g. bug 663800.

I see no reason why fixing that bug should require us to separate timelines.

> A single global timer would fix this particular bug more easily, but it
> wouldn't fix bug 663800, and wouldn't give us other benefits of refresh
> driver control such as throttling image animation in hidden tabs.

Sharing animated images across tabs is an edge case, and adding a huge amount of new code just to make that work more efficiently seems like the wrong trade-off to me.

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Sharing animated images across tabs is an edge case

It's the "hey, I have a few tabs open on this forum where all these people are using animated avatars and smilies" use case....

Comment 23

[Joe Drew (not getting mail)]

(In reply to comment #22)
> > Sharing animated images across tabs is an edge case
> 
> It's the "hey, I have a few tabs open on this forum where all these people
> are using animated avatars and smilies" use case....

True, but I still think it's pretty niche. I might be wrong.

Keep in mind that, aside from drawing (which we can/do hopefully throttle on background tabs with nsRefreshDriver), sharing animation state is _less_ work.

You just register for all your nsRefreshDrivers, then update to the time given by the callback. In the common case, the background tab won't do anything other than trigger invalidation.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #21)
> > Sharing timelines is a bug, e.g. bug 663800.
> 
> I see no reason why fixing that bug should require us to separate timelines.

AFAICT the simplest approach is to separate timelines. If you have a better suggestion, I'm open to it.

> > A single global timer would fix this particular bug more easily, but it
> > wouldn't fix bug 663800, and wouldn't give us other benefits of refresh
> > driver control such as throttling image animation in hidden tabs.
> 
> Sharing animated images across tabs is an edge case, and adding a huge
> amount of new code just to make that work more efficiently seems like the
> wrong trade-off to me.

I don't think we need a huge amount of new code to do this separation.

I wasn't thinking about "sharing animated images across tabs". Suppose we have a single animated image in a hidden tab. How are you proposing we throttle its animation?

Comment 25

[Daniel Holbert [:dholbert]]

(In reply to comment #24)
> Suppose we have a single animated image in a hidden tab.
> How are you proposing we throttle its animation?

We already do something related for *closed* tabs -- we keep count of "mAnimationConsumers" on Image (RasterImage's superclass), and that count gets decremented in OnPageHide, via nsDocument::SetImagesNeedAnimating().  (Then we stop animation altogether when the count hits 0.)

So, we could potentially use a similar strategy to throttle in hidden tabs -- add a new member-var "mForegroundAnimationConsumers", and throttle (but not stop) animations whenever that hits 0.

(I'm not necessarily advocating that we do this -- just pointing it out as one way to throttle in background tabs without switching to per-document timelines.)

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, we can do that.

I just feel that we're spending energy trying to come up with short-cuts to avoid fixing the problem right, for no valid reason I can see.

Comment 27

[Joe Drew (not getting mail)]

(In reply to comment #24)
> (In reply to comment #21)
> > > Sharing timelines is a bug, e.g. bug 663800.
> > 
> > I see no reason why fixing that bug should require us to separate timelines.
> 
> AFAICT the simplest approach is to separate timelines. If you have a better
> suggestion, I'm open to it.

Significantly simpler than that is to register for every nsRefreshDriver in every tab you're in (happens automatically via nsImageFrame or whatever), then listening for the current time (i.e., the refresh driver callback). When the time changes such that you need to change your frame, RasterImage does its compositing, then sends invalidations as normal.

All that code needs to be written anyways. The difference between this and comment 11 is that this doesn't involve writing new imgRequest implementations, doesn't require changing RasterImage to be able to share imgFrames, etc.

The only downside to this is that we don't throttle invalidations on background tabs. That's not an enormous problem; we probably need to throttle painting on background tabs anyways, because js can paint at whatever time it wants.

If, at some later time (or even now) we decide we desperately want separate timelines on separate tabs, we should cache animated images per-document instead of writing all that complicated code.

Comment 28

[Joe Drew (not getting mail)]

To be clear, I read roc's "the simplest approach is to separate timelines" as "the simplest approach to fix _this bug_ is to separate timelines". If that's not what roc was saying, then I was responding to something else.

But comment 27 is still how I think the simplest way to solve this bug would go.

Comment 29

[Boris Zbarsky [:bzbarsky]]

> we probably need to throttle painting on background tabs anyways

We don't paint them at all.  But right now just the actual Invalidate() call can be pretty expensive....

Comment 30

[Daniel Holbert [:dholbert]]

When I load the URL from comment 0 in a foreground tab and let it load fully, I get 99% Firefox CPU usage and 15-20% Xorg CPU usage.  When I switch tabs, both drop to 3-5%.  (And when I close the tab, Firefox drops to 0-1%.)

So, the invalidates aren't free, but relative to painting, they're pretty cheap...

Comment 31

[Joe Drew (not getting mail)]

As a tangent, why do we do invalidations in background tabs? Why not just ignore them and invalidate the whole page when we bring the tab to the front?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #27)
> Significantly simpler than that is to register for every nsRefreshDriver in
> every tab you're in (happens automatically via nsImageFrame or whatever),
> then listening for the current time (i.e., the refresh driver callback).
> When the time changes such that you need to change your frame, RasterImage
> does its compositing, then sends invalidations as normal.

OK. This does require imagelib changes to not use a timer and instead require explicit time advance via some new API; I assume you're OK with that?

There is also the complicating issue that we'll need to track, for each use of an image, the time offset between the refresh driver's time and the image's timeline. That code would go away if/when we switch to one animated image timeline per image instance or document.

I still don't think that plan is optimal, but it is satisfactory, so go for it.

> If, at some later time (or even now) we decide we desperately want separate
> timelines on separate tabs, we should cache animated images per-document
> instead of writing all that complicated code.

We can't do that since we don't know an image is animated until we've loaded it, right?

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #31)
> As a tangent, why do we do invalidations in background tabs? Why not just
> ignore them and invalidate the whole page when we bring the tab to the front?

We could. Checking whether content is in a background tab has a cost, though, so we wouldn't want to do that all time.

Comment 34

[Joe Drew (not getting mail)]

(In reply to comment #32)
> OK. This does require imagelib changes to not use a timer and instead
> require explicit time advance via some new API; I assume you're OK with that?

Yes, of course. That's why I suggested it :)

> > If, at some later time (or even now) we decide we desperately want separate
> > timelines on separate tabs, we should cache animated images per-document
> > instead of writing all that complicated code.
> 
> We can't do that since we don't know an image is animated until we've loaded
> it, right?

That's a complicating factor, but it's pretty easily worked around without any new classes.

Comment 35

[Daniel Holbert [:dholbert]]

(In reply to comment #32)
> OK. This does require imagelib changes to not use a timer and instead
> require explicit time advance via some new API; I assume you're OK with that?

(This is like "::RefreshTick" from Comment 11)

> There is also the complicating issue that we'll need to track, for each use
> of an image, the time offset between the refresh driver's time and the
> image's timeline. That code would go away if/when we switch to one animated
> image timeline per image instance or document.

Alternately, we could make RasterImage completely ignore the timestamp passed down from RefreshDriver, and instead use PR_Now() to find out how much time has passed since its last tick -- right?

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could, but I would rather not do that. If we use the refresh driver's time, we get the added benefit of keeping animated images in sync with other animations on the page, at least if it's not shared with other pages. A small benefit, to be sure, but worth having if we can get it easily.

Comment 37

[Daniel Holbert [:dholbert]]

Actually, from looking at the RefreshDriver impl, it looks like the timestamps it passes in are from Timestamp::Now() (that is -- not tied to any document).  

So the image should be able to directly use the time that's passed into ::RefreshTick (or whatever it's called), without caring about which document it came from.

Comment 38

[Timothy Nikkel (:tnikkel)]

(In reply to comment #33)
> (In reply to comment #31)
> > As a tangent, why do we do invalidations in background tabs? Why not just
> > ignore them and invalidate the whole page when we bring the tab to the front?
> 
> We could. Checking whether content is in a background tab has a cost,
> though, so we wouldn't want to do that all time.

We do this check in the root frame of every document right now. So the invalidation does get dropped, but it first has to make its way through the frame tree of one document first.

We of course have to invalidate the whole visible part of the page when it becomes visible.

Comment 39

[Scott Johnson (:jwir3)]

Attachment: Patch 0 - Interface changes for b666446

First in a series of patches to fix this bug. This patch is an API change for imgIContainer - probably needs super review.

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 545294 [details] [diff] [review]
Patch 0 - Interface changes for b666446

>diff --git a/modules/libpr0n/public/imgIContainer.idl b/modules/libpr0n/public/imgIContainer.idl
>+[ref] native timestamp(mozilla::TimeStamp);

Seems like it'd be clearest if we made the IDL typename match the native typename here.  So, s/timestamp/Timestamp/

>+  /** 
>+    * Sends a signal indicating that this imgIContainer has been triggered by
>+    * a layout frame to refresh itself. Likely this should only be called from
>+    * within nsImageFrame or objects of similar type.
>+    */
>+  [notxpcom] void refreshTick([const] in timestamp t);
>+    

Nix whitespace-on-blank-line at the end there.  (there are some stray space characters)

Also, the header comment should probably mention animation.  Perhaps replace "to refresh iteself" with "to update its internal animation state".

Also, prefix function-arguments with "a", and use a slightly-more-descriptive arg name, e.g. "aTime".

>+// [notxpcom] void refreshTick([const] in timestamp t);
>+void RasterImage::RefreshTick(const mozilla::TimeStamp& t)
>+{
>+  // TODO: Implement me
>+}
>+

Add newline after "void", to match the prevailing mozilla style. Same with in VectorImage.cpp

>diff --git a/modules/libpr0n/src/VectorImage.cpp b/modules/libpr0n/src/VectorImage.cpp
>@@ -166,16 +166,17 @@ SVGDrawingCallback::operator()(gfxContex
>   // Clip to aFillRect so that we don't paint outside.
>   aContext->NewPath();
>   aContext->Rectangle(aFillRect);
>   aContext->Clip();
> 
>   gfxContextMatrixAutoSaveRestore contextMatrixRestorer(aContext);
>   aContext->Multiply(gfxMatrix(aTransform).Invert());
> 
>+  // [notxpcom] void refreshTick([const] in timestamp t);
> 
>   nsPresContext* presContext = presShell->GetPresContext();

I think that added line (inside of SVGDrawingCallback::operator()) isn't supposed to be there.

Comment 41

[Randell Jesup [:jesup] (needinfo me)]

For historical reference on performance and background-tab animation, see bug 125025, bug 125137, and especially bug 120154 (which is still open, and fairly directly relevant if out-of-date).

Comment 42

[Scott Johnson (:jwir3)]

Attachment: Patch 0v2 - Interface changes for b666446

Fixed issues described by dholbert in review yesterday.

Comment 43

[Scott Johnson (:jwir3)]

Attachment: Patch 0v3 - Interface changes for b666446

Comment 44

[Scott Johnson (:jwir3)]

Attachment: Patch 0v4 - Interface changes for b666446

Sorry about the review spam. I accidentally posted only the changes to the .idl file. This file is now the patch that actually fixes the review issues presented by dholbert.

Comment 45

[Daniel Holbert [:dholbert]]

Comment on attachment 545433 [details] [diff] [review]
Patch 0v4 - Interface changes for b666446

>+++ b/modules/libpr0n/src/RasterImage.cpp
>+// [notxpcom] void refreshTick([const] in timestamp t);
>+void
>+RasterImage::RefreshTick(const mozilla::TimeStamp& aTime)
>+{
>+  // TODO: Implement me
>+}

Nit: The comment there is now out of date ("timestamp t" --> "Timestamp aTime") Applies to the same spot in VectorImage.cpp, too.

Actually -- I think most of the impl-header-comments in these files are just directly copy/pasted from $OBJDIR/dist/include/imgIContainer.h, from the corresponding chunks underneath the 
  "Use the code below as a template for the implementation class for this interface"
comment.  (note that this .h file is auto-generated from the .idl file)

Probably best to do that, to be sure to match the style of the other header-comments in this file. (switching out // for /**/, too)  (super-picky, sorry :))

>+++ b/modules/libpr0n/src/VectorImage.cpp
>   gfxContextMatrixAutoSaveRestore contextMatrixRestorer(aContext);
>   aContext->Multiply(gfxMatrix(aTransform).Invert());
> 
>-
>   nsPresContext* presContext = presShell->GetPresContext();
>   NS_ABORT_IF_FALSE(presContext, "pres shell w/out pres context");
> 

Nit: This newline-deletion looks unrelated... However, it's a good change (and doesn't break blame), so I won't complain if you keep it in. :)

So, r=dholbert with the cpp-file comment-tweaks.

Comment 46

[Daniel Holbert [:dholbert]]

(In reply to comment #45)
> Actually -- I think most of the impl-header-comments in these files are just
> directly copy/pasted from $OBJDIR/dist/include/imgIContainer.h

(also, per IRL conversation, you'll also need to use the NS_IMETHODIMP(void) or whatever from imgIContainer.h, at the beginning of the function impls)

Comment 47

[Scott Johnson (:jwir3)]

Attachment: Patch 0v4 (v5) - Interface changes for b666446 [r=dholbert]

Posting patch as reviewed by dholbert, requesting second r in a second comment.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 545516 [details] [diff] [review]
Patch 0v4 (v5) - Interface changes for b666446 [r=dholbert]

Was there a reason to not call the API willRefresh, since this is basically forwarding on notifications from the refresh driver?

Other than that, looks fine.

Comment 49

[Daniel Holbert [:dholbert]]

(In reply to comment #48)
> Was there a reason to not call the API willRefresh, since this is basically
> forwarding on notifications from the refresh driver?

I'd recommended that we use a different function name, to prevent confusion over whether imgIContainer impls are expected to implement nsARefreshObserver (and also because these calls will be coming from multiple refresh drivers simultaneously).  That was possibly a silly concern on my part, though.  I don't have strong feelings on it at this point.

Comment 50

[Boris Zbarsky [:bzbarsky]]

OK, I buy that argument.  How about refreshRequested then?  My issue with "refreshTick" is that it sounds like an order to refresh a tick, whatever that means....

Comment 51

[Joe Drew (not getting mail)]

Comment on attachment 545516 [details] [diff] [review]
Patch 0v4 (v5) - Interface changes for b666446 [r=dholbert]

Review of attachment 545516 [details] [diff] [review]:
-----------------------------------------------------------------

I'd also love to see your in-progress patches for implementing this interface.

::: modules/libpr0n/public/imgIContainer.idl
@@ +71,5 @@
>  native gfxGraphicsFilter(gfxPattern::GraphicsFilter);
>  [ref] native nsIntRect(nsIntRect);
>  [ref] native nsIntSize(nsIntSize);
>  [ptr] native nsIFrame(nsIFrame);
> +[ref] native Timestamp(mozilla::TimeStamp);

nit: TimeStamp

@@ +271,5 @@
> +    * a layout frame to update its internal animation state. Likely this
> +    * should only be called from within nsImageFrame or objects of similar
> +    * type.
> +    */
> +  [notxpcom] void refreshTick([const] in Timestamp aTime);

I'm not choosy about the name, but I prefer it be in the active tense; requestRefresh? I'm also ok with willRefresh, timerTick, updateAnimation, etc.

Comment 52

[Scott Johnson (:jwir3)]

> I'd also love to see your in-progress patches for implementing this
> interface.
Sure.
I have patches available at:
http://hg.mozilla.org/users/sjohnson_mozilla.com/patches
The one I'm currently working on, b666446-nsImageFrame-impl, I believe includes implementations for RefreshTick() (or whatever we're going to call it) in RasterImage and VectorImage (although VectorImage is still just a stub).

> nit: TimeStamp
Sure, I'll make that modification.

> I'm not choosy about the name, but I prefer it be in the active tense;
> requestRefresh? I'm also ok with willRefresh, timerTick, updateAnimation,
> etc.

I agree. How about requestRefresh()? Does that make sense to everyone?

Comment 53

[Scott Johnson (:jwir3)]

> I'd also love to see your in-progress patches for implementing this
> interface.

Sorry, I re-read this and realized it might not be entirely clear. I just gave a link to my patch queue so that you can take a look at my work as I'm doing it. I will post work-in-progress patches once I get to a state where they are working somewhat well. 

Also, I just reviewed that patch and realized the implementations for RasterImage and VectorImage are not in that patch, so I'll post a patch once the implementations move from my head to source files. :)

Comment 54

[Boris Zbarsky [:bzbarsky]]

requestRefresh is fine by me.

Comment 55

[Scott Johnson (:jwir3)]

Attachment: Patch 0 (v6) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]

Patch with the interface conforming to review comments by dholbert, JOEDREW!, and bz. The RefreshTick() function has also been changed to RequestRefresh().

Comment 56

[Joe Drew (not getting mail)]

(In reply to comment #52)

> http://hg.mozilla.org/users/sjohnson_mozilla.com/patches
> The one I'm currently working on, b666446-nsImageFrame-impl, I believe
> includes implementations for RefreshTick() (or whatever we're going to call
> it) in RasterImage and VectorImage (although VectorImage is still just a
> stub).

You probably haven't committed/pushed that change yet, because I couldn't see any imagelib stuff in that patch :(

Comment 57

[Daniel Holbert [:dholbert]]

(In reply to comment #56)
> You probably haven't committed/pushed that change yet, because I couldn't
> see any imagelib stuff in that patch :(

(yup, see second half of comment 53 :))

Comment 58

[Joe Drew (not getting mail)]

Er, yes.

Comment 59

[Scott Johnson (:jwir3)]

Attachment: Patch 1 - Implementation for nsImageFrame (work in progress)

This is work in progress for patch 1, if you'd like to look it over. Not requesting review just yet, because it will probably change.

Comment 60

[Scott Johnson (:jwir3)]

Attachment: Patch 2 - Implementation for RasterImage (work in progress)

This is work in progress for patch 2, if you'd like to look it over. Not requesting review just yet, because it will probably change.

Comment 61

[Scott Johnson (:jwir3)]

I've posted a couple of patches for work in progress. I'm currently in the process of debugging what appears to be an issue loading the GIF images with the RasterImage implementation.

Comment 62

[Scott Johnson (:jwir3)]

So it appears that the test case described in comment 1 is giving me troubles now (appears to be network troubles - it's unavailable even if I attempt to Save as complete' to the page). As such, I'm posting another test case from bug 595671, downloaded and wrapped up as a .zip file.

Comment 63

[Scott Johnson (:jwir3)]

Attachment: Test Case 1 - Animated Gifs

This is a test case for this bug.

Comment 64

[Scott Johnson (:jwir3)]

Attachment: Test Case 0 - Original Test Case

Original test case from comment 1.

Comment 65

[Scott Johnson (:jwir3)]

Disregard comment 62. I was able to get the web page to load now, and performed a 'save as complete' on it. It's attached in attachment 545998 [details].

Comment 66

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v2) - Implementation for nsImageFrame (work in progress)

Modified a comment that had an incorrect additional word at the end of the line.

Comment 67

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v3) - Implementation for nsImageFrame (work in progress)

Moved some stuff over from Patch 2 to make Patch 1 more cohesive and self-contained.

Comment 68

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v2) - Implementation for RasterImage (work in progress)

Moved some items from Patch 2 to Patch 1 to make Patch 1 more self-contained and cohesive.

Comment 69

[Scott Johnson (:jwir3)]

I'm going to be updating the RasterImage patch to add a couple of details that I missed the first time around, also to rebase against the patch for bug 672225.

Also, I thought I would share some basic performance testing I did for this bug, after the patch was applied.  I noticed a sharp increase in the number of calls to RasterImage::Draw() on 7/14/11, after applying the patch: 

Chat site without patch (~2min):
Draw Frame Count: 17873

Chat site with patch (~2min):
Draw Frame Count: 86227

After speaking with dholbert about this, we agreed that this might not necessarily be bad, because if the number of actual expose events is reduced, it might indicate that we're just getting more frames through the pipeline, thus increasing the frame rate (which is the goal to begin with anyway). To verify this, I put another set of print statements inside of the OnExposeEvent() callback, as before, to produce these results:

Chat Site w/ 1 minute run time:
Expose Counter: 1470
Draw Frame Count: 4300

Chat Site w/ 2 minutes run time:
Expose Counter: 5040
Draw Frame Count: 23200

Finally, we agreed that if this were the case, then the counts for a single image should be about the same number of calls to Draw() and OnExposeEvent (relative to one another). I used one of the images from the chat site and simply loaded JUST that image).

Chat Site single image icon without patch (~2min):
Draw Frame Count: 7200
Expose Counter: 1210

Chat Site single image icon with patch (~2min):
Draw Frame Count: 6900
Expose Counter: 1160

Additionally, the performance with the patch has visibly increased, and the CPU load has gone down from 100% usage of at least 1 core to roughly 67% usage of a least one core. So, it seems clear that this has given us about a 33% speedup, at least in RasterImage.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds like you're on the right track, but I think the best comparison would be expose event rate before and after your patch.

Comment 71

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v4) - Implementation for nsImageFrame

Made some minor adjustments to the nsImageFrame implementation for this bug from the WiP version.

Comment 72

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v3) - Implementation for RasterImage

Patch that fixes the RasterImage.h/.cpp files for animated gif performance issues.

Comment 73

[Daniel Holbert [:dholbert]]

Comment on attachment 547521 [details] [diff] [review]
Patch 1 (v4) - Implementation for nsImageFrame

>+nsImageRefreshObserver::nsImageRefreshObserver(nsImageFrame* aParent)
>+{
>+  Register(aParent);
>+}

This would be nicer as:
  : mParent(aParent)
  {
    NS_ABORT_IF_FALSE(mParent, "nsImageRefreshObserver requires an image frame");
    Register();
  }

>+void
>+nsImageRefreshObserver::WillRefresh(mozilla::TimeStamp aTime)
>+{
>+  if (!mFrame) {
>+    // we don't want to dereference a null ptr
>+    return;
>+  }

I don't think it's worth null-checking mFrame here, though it'd be worth adding an NS_ABORT_IF_FALSE just to make clear the assumption that it's non-null. There's no sane way that we'd get here with a null value (since we unregister from refresh-driver & null out mFrame at the same spot), so the check isn't really necessary.  Moreover, if something breaks horribly and mFrame ends up being null somehow, then we'll just crash on a null-deref, which is relatively safe.

>+void
>+nsImageRefreshObserver::Register(nsImageFrame* aFrame)
>+{
>+  if (!aFrame) {
>+    // we can't register a null frame
>+    return;
>+  }
>+  mFrame = aFrame;

As noted above, it'd be simpler to set |mFrame| in the constructor init list.  Also as noted above, no need for the null-check.

>+void
>+nsImageRefreshObserver::Deregister()
>+{
>+  if (!mFrame) {
>+    return;
>+  }

As noted above, no need for the null-check.

>+  nsPresContext* presContext = mFrame->PresContext();
>+  presContext->PresShell()->RemoveRefreshObserver(this,
>+                                                  mozFlushType::Flush_Display);
>+  mFrame = nsnull;

Maybe add a comment explaining why we care about nulling this out, e.g.:
  // mFrame is being destroyed -- drop our reference to it, so we don't deref
  // a deleted pointer if we somehow outlive it & get callbacks after it's deleted
  // (highly unexpected)

>+  NS_ABORT_IF_FALSE(mRefCnt != 1, "Multiple references exist to an image refresh observer.");

Nit: no period at end of assertion messages.
(to prevent ".:" grossness in "this is a message.: path/to/file.cpp")

>+++ b/layout/generic/nsImageFrame.h
>+class nsImageRefreshObserver: public nsARefreshObserver
>+{

Add a short comment above this helper-class to explain what it's used for.

>+  // Handle to the refresh driver observation helper class
>+  nsRefPtr<nsImageRefreshObserver> mRefreshObs;
>+

Add a comment explaining why this is a nsRefPtr (rather than a nsAutoPtr, which we would normally use for one-off helper-objects like this).
e.g. 
 // Refcounted because the nsRefreshDriver temporarily AddRef's this object
 // during each refresh ping.

Comment 74

[Daniel Holbert [:dholbert]]

(In reply to comment #73)
> I don't think it's worth null-checking mFrame here, though it'd be worth
> adding an NS_ABORT_IF_FALSE just to make clear the assumption that it's
> non-null.

(but if you'd like to keep the null-check in WillRefresh, I wouldn't object as long as you add an NS_ERROR or NS_ABORT_IF_FALSE to accompany it.)

Comment 75

[Daniel Holbert [:dholbert]]

Comment on attachment 547522 [details] [diff] [review]
Patch 2 (v3) - Implementation for RasterImage

>+++ b/layout/generic/nsImageFrame.cpp
>@@ -213,17 +213,17 @@ nsImageRefreshObserver::Deregister()
>-  NS_ABORT_IF_FALSE(mRefCnt != 1, "Multiple references exist to an image refresh observer.");
>+  NS_ABORT_IF_FALSE(mRefCnt == 1, "Multiple references exist to an image refresh observer.");
> }

Looks like this chunk belongs in the previous patch.

>+nsresult
>+RasterImage::AdvanceFrame(imgIContainerObserver* aObserver, TimeStamp aTime)
>+{

This probably wants to return void, rather than nsresult.  The only value it returns right now is NS_OK.

>+  if (mFrames.IsEmpty()) {
>+    return NS_OK;
>+  }

I'm not sure it's makes sense to check for this.  Note that when we get here, we've already evaluated:
   mFrames[mAnim->currentAnimationFrameIndex];
in the caller (RequestRefresh), so it looks like we're already assuming that mFrames is nonempty.
I'm not sure if that's an valid assumption, but either way, it seems like we shouldn't be getting as far as AdvanceFrame if we have no frames.
So perhaps convert this into an ASSERTION or ABORT_IF_FALSE?

>+  imgFrame* nextFrame = nsnull;
>+  PRUint32 previousFrameIndex = mAnim->currentAnimationFrameIndex;
>+  PRUint32 nextFrameIndex = mAnim->currentAnimationFrameIndex + 1;

It looks like this only supports going forward 1 frame at a time, regardless of how long it's been since the last one.  Looks like this still needs fixing to support jumps of multiple frames at a time, right?

Also -- I think this would be clearer if we removed the "previous" / "prev" terminology, and instead dealt only with "current frame" and "next frame".  (particularly given that you wait until the last second to do the "current = next" assignment, which is great)  I'd suggest s/previous/current/ here, and also a chunk later on in this function "imgFrame *prevFrame = mFrames[previousFrameIndex]" would want s/prevFrame/curFrame/.

>+  PRInt32 timeout = 0;
>+
>+  // Figure out if we have the next full frame. This is more complicated than
>+  // just checking for mFrames.Length() because decoders append their frames
>+  // before they're filled in.
>+  NS_ABORT_IF_FALSE(mDecoder || nextFrameIndex <= mFrames.Length(),
>+                    "How did we get 2 indicies too far by incrementing?");

s/indicies/indices/

>+  bool haveFullNextFrame = !mDecoder || nextFrameIndex < mDecoder->GetCompleteFrameCount();
>+
>+  // If we're done decoding the next frame, go ahead and display it now and
>+  // reinit the timer with the next frame's delay time.
>+  if (haveFullNextFrame) {
>+    if (mFrames.Length() == nextFrameIndex) {
>+      // End of Animation
>+
>+      // If animation mode is "loop once", it's time to stop animating
>+      if (mAnimationMode == kLoopOnceAnimMode || mLoopCount == 0) {
>+        mAnimationFinished = PR_TRUE;
>+        EvaluateAnimation();
>+        return NS_OK;
>+      } else {

Drop the "else" here - unnecessary after a return statement (and it confuses some compilers, IIRC).

>+    // Change frame and announce it
>+    if (NS_FAILED(DoComposite(&frameToUse, &dirtyRect, prevFrame,
>+                              nextFrame, nextFrameIndex))) {
>+      // something went wrong, move on to next
>+      NS_WARNING("RasterImage::AdvanceFrame(): Composing Frame Failed\n");

No "\n" needed in warning messages

>+      nextFrame->SetCompositingFailed(PR_TRUE);
>+      mAnim->currentAnimationFrameIndex = nextFrameIndex;
>+      return NS_OK;
>+    } else {
>+      nextFrame->SetCompositingFailed(PR_FALSE);

Drop else after return here, too.

> RasterImage::RequestRefresh(const mozilla::TimeStamp& aTime)
> {
>+  this->ensureAnimExists();
>+  if (!mAnimating || !ShouldAnimate()) {
>+    return;
>+  }
A few things on ensureAnimExists:
 (a) no need for |this->|
 (b) We only want to call it after we've determined that we're animating.
 (c) It looks like ensureAnimExists thinks it can fail (that is, we have failure-checks for it in other places) -- however, infallible-new means it really can't fail.  To avoid confusion about where/whether we need to failure-check it, perhaps we should just make ensureAnimExists() return void? (and capitalize it to follow convention? I think it's the only non-capitalized method in RasterImage.)

Maybe (c) could happen in an separate patch (or separate bug).

>+  NS_ABORT_IF_FALSE(mAnim,
>+                    "RequestRefresh(): Animation object cannot be null");

This ABORT_IF_FALSE doesn't really make sense, given that we just called ensureAnimExists().  If you want to keep it, maybe change message to "ensureAnimExists lied!", to be clearer about why we expect the asserted condition to be true?

>+  // only advance the frame if the current time is greater than or
>+  // equal to the last frame time + the frame's offset
>+  imgFrame* currentFrame = mFrames[mAnim->currentAnimationFrameIndex];
>+  TimeStamp lastFrameTime = mAnim->currentAnimationFrameTime;
>+  PRInt64 timeout = currentFrame->GetTimeout();
>+  TimeDuration durationOfTimeout = TimeDuration::FromMilliseconds(timeout);
>+  TimeStamp timePlusTimeout = lastFrameTime + durationOfTimeout;

As noted for "prev" in AdvanceFrame:  the "current" vs. "last" distinction is a little confusing here, too - I think this would be clearer if it were just in terms of "current frame" and "next frame", with no mention of "last".

So, maybe replace the final 4 lines quoted above with:
    TimeStamp nextAnimationFrameTime = mAnim->currentAnimationFrameTime +
      TimeDuration::FromMilliseconds(currentFrame->GetTimeout());

>+    nsCOMPtr<imgIContainerObserver> observer(do_QueryReferent(mObserver));
>+    if (!observer) {
>+      // the imgRequest that owns us is dead, we should die now too.
>+      NS_ABORT_IF_FALSE(mAnimationConsumers == 0,
>+          "If no observer, should have no consumers");
>+      if (mAnimating) {
>+        StopAnimation();
>+      }
>+  
>+    this->AdvanceFrame(observer, aTime);

This chunk is problematic for a few reasons (though I think a good chunk of the weirdness predates this patch, and comes from ::Notify()).

Issue #1: We assert that mAnimationConsumers is 0 -- but if that were true, then Image::ShouldAnimate() would have returned false at the beginning of this method, and we never would have gotten here.  So the assertion is bogus.
Issue #2: We check 'mAnimating' -- but that's guaranteed to be true, because we already checked it at the beginning of this method.
Issue #3: Our clients (e.g. nsImageFrames) presumably had to use the imgRequest in order to look up the imgIContainer in order to call this method in the first place -- so if that were dead, I don't think it'd be possible for us to even get here. (?)
Issue #4: We currently we go ahead and call AdvanceFrame even though |observer| is null, which will crash.

I think this chunk can be reduced to:
   if (!observer) {
     NS_ERROR("Refreshing image after its imgRequest is gone");
     StopAnimation();
     return;
   }
(I believe that addresses all the issues I brought up above.)

>   /**
>+   * Advances the animation. Typically, this will advance a single frame, but it
>+   * may advance multiple frames in the case where a frame's length is <= 1
>+   * refresh driver "tick" - ~ 1/60th of a second

Better not to be this specific -- the 60hz figure is only true for foreground tabs, and even there it's not set in stone. (It's configurable in about:config, actually.)

Probably best not to mention any particular frequency here - instead, say something like:
   This may happen if we have infrequently-"ticking" refresh drivers (e.g. in background
   tabs), or extremely short-lived animation frames.

>+   *
>+   * @param aObserver the imgIContainerObserver to notify after the frame has
>+   *                 been changed.

Indentation issue there -- add space before 'been'.

>+   * @param aTime the time that the animation should advance to. this will
>+   *              typically be <= TimeStamp::Now().

The <= Now() statement there is probably worth asserting in the implementation, actually.  Also, capitalize "this"

Comment 76

[Daniel Holbert [:dholbert]]

>+    this->AdvanceFrame(observer, aTime);

Also: No need for |this->| there.

Comment 77

[Scott Johnson (:jwir3)]

Hi Daniel:

Thanks for the feedback - I'll make these changes and then repost.

Comment 78

[Scott Johnson (:jwir3)]

> >+  imgFrame* nextFrame = nsnull;
> >+  PRUint32 previousFrameIndex = mAnim->currentAnimationFrameIndex;
> >+  PRUint32 nextFrameIndex = mAnim->currentAnimationFrameIndex + 1;
> 
> It looks like this only supports going forward 1 frame at a time, regardless
> of how long it's been since the last one.  Looks like this still needs
> fixing to support jumps of multiple frames at a time, right?

Yes, I think you're right on this. After playing with this a bit, I noticed that we already have timing calculations in RequestRefresh(), and calculating how many frames we should advance in AdvanceFrame() is going to duplicate this timing calculation. Additionally, it depends on mAnim->currentAnimationFrameTime, which is only set in AdvanceFrame. So, it seems like a bit of a chicken-and-egg problem. 

I think I'm going to tackle this problem by implementing AdvanceFrame() to ONLY advance a single frame, and then in RequestRefresh force it to potentially call AdvanceFrame() multiple times if necessary to advance more than one frame. This is more overhead, although I'm not convinced it will be a significant amount of additional overhead, and I think the case where we're advancing more than one frame in a single refresh driver tick will be somewhat less common than the single-frame-advancement case. 

Thoughts?

Comment 79

[Scott Johnson (:jwir3)]

> which is only set in AdvanceFrame. So, it
> seems like a bit of a chicken-and-egg problem. 

Also, by this I mean that when I added the timing calculation into AdvanceFrame, I'm getting issues with mAnim->currentAnimationFrameTime not being initialized prior to the first frame advancement, which causes a number of problems.

Comment 80

[Daniel Holbert [:dholbert]]

(In reply to comment #78)
> After playing with this a bit, I noticed
> that we already have timing calculations in RequestRefresh() and
> calculating how many frames we should advance in AdvanceFrame() is going to
> duplicate this timing calculation.

Not necessarily -- RequestRefresh() only checks "Are we past the end of the current frame", whereas AdvanceFrame could be checking "Ok, what frame _are_ we in?"  You could even pass in "aTimePastEndOfCurrentFrame" instead of "aTime" as an argument to AdvanceFrame, if you like.  But your suggestion works too, per below.

(In reply to comment #78)
> I think I'm going to tackle this problem by implementing AdvanceFrame() to
> ONLY advance a single frame, and then in RequestRefresh force it to
> potentially call AdvanceFrame() multiple times if necessary to advance more
> than one frame.

Yeah, that's probably the simplest way forward for now.  If you're going to do that, though, I think we should only send the FrameChanged notifications once.  If we have a bunch of listeners, I think the duplicate invalidations could potentially be expensive (and either way they're definitely definitely redundant/silly).  So I think we need to move that FrameChanged call out to RequestRefresh().  Perhaps AdvanceFrame() can return a boolean indicating whether an invalidation is required or not (and RequestRefresh would honor the boolean returned by its final call to AdvanceFrame()).

(In reply to comment #79)
> I'm getting issues with mAnim->currentAnimationFrameTime not
> being initialized prior to the first frame advancement, which causes a
> number of problems.

Hmm -- it seems like that could cause issues in your attached patch, too -- I don't see where that ever gets initialized.  I think you'll need to start that out with some sentinel value (e.g. 0, #defined as FRAME_TIME_UNINITIALIZED or something?), and have a check for that to handle the first-frame case.

Comment 81

[Scott Johnson (:jwir3)]

> If you're going to
> do that, though, I think we should only send the FrameChanged notifications
> once.  If we have a bunch of listeners, I think the duplicate invalidations
> could potentially be expensive (and either way they're definitely definitely
> redundant/silly).  So I think we need to move that FrameChanged call out to
> RequestRefresh().  Perhaps AdvanceFrame() can return a boolean indicating
> whether an invalidation is required or not (and RequestRefresh would honor
> the boolean returned by its final call to AdvanceFrame()).

Hm, yes, I hadn't anticipated this initially. It does seem like a waste to send the FrameChanged() notifications multiple times. And it seems logical to have AdvanceFrame() advance a single frame. On the other hand, it seems equally logical to have the function AdvanceFrame() being fairly cohesive - which entails that it should send the notification, not the RequestRefresh() function. (Am I the only one that thinks this is more mentally cohesive to have the FrameChanged notification happen in AdvanceFrame()?)
 
> Hmm -- it seems like that could cause issues in your attached patch, too --
> I don't see where that ever gets initialized.  I think you'll need to start
> that out with some sentinel value (e.g. 0, #defined as
> FRAME_TIME_UNINITIALIZED or something?), and have a check for that to handle
> the first-frame case.

Yeah, I'm not sure why I didn't hit it before. I definitely thought this should have been a problem, but I wasn't seeing the same segfault I am seeing after I duplicated the timing calculations in the AdvanceFrame() call. It seems that mAnim->currentAnimationFrameTime is null at some point, which is causing a call to TimeStamp.operator+ (TimeDuration&) to fail an assertion. I *did* notice these assertion failures in the previous patch, but I didn't connect that they were coming from the TimeStamp::operator+ function until now. However, now it's also throwing a segfault, so I'm looking into why this is causing a problem like this. Perhaps when I have a bit more information, I can give a better suggestion as to how to proceed.

Comment 82

[Randell Jesup [:jesup] (needinfo me)]

>+      // If animation mode is "loop once", it's time to stop animating
>+      if (mAnimationMode == kLoopOnceAnimMode || mLoopCount == 0) {
>+        mAnimationFinished = PR_TRUE;
>+        EvaluateAnimation();
>+        return NS_OK;
>+      } else {
>
>Drop the "else" here - unnecessary after a return statement (and it confuses some compilers, IIRC).

Any compiler that confuses should be recycled into random bits.  :-) Seriously, that's simply fundamental correctness; we shouldn't use any compiler that fubars that (and I can't imagine any non-toy compiler does).

If that's a style issue, I'd say "what's the prevailing style in the file/module?"  I do stuff like that when it (IMO) aids readability; I don't have a strong opinion pro or con to that - I personally wouldn't flag it in a review, but I wasn't the reviewer here.

Comment 83

[Scott Johnson (:jwir3)]

dholbert and I were speaking on IRC (in order to minimize a giant slough of comments back and forth), and we came to the agreement that the following is a good procedure to handle the AdvanceFrame/RequestRefresh problem:

  1) Implement AdvanceFrame to advance a _single_ frame, but without the notification.
  2) Extend the implementation of RequestRefresh() to calculate if a frame advancement is necessary, and then advance frames incrementally using AdvanceFrame() until it either a) fails or b) reaches the desired time. If at least one frame has been successfully advanced, then it will trigger an invalidate notification.

This saves us from having to do the timing calculations multiple times. AdvanceFrame is pretty cheap without the FrameChanged() call, so I think we're pretty safe in that respect. It does, however, calculate whether or not the next frame is currently decoding (which is when it would fail if the next frame can't be shown). In that case, we still want to trigger an update notification if we have had at least one frame changed since the start of RequestRefresh().

Just wanted to give you folks a heads-up as to what we were discussing, in the event that you have comments/suggestions.

Comment 84

[Daniel Holbert [:dholbert]]

(In reply to comment #82)
> >Drop the "else" here - unnecessary after a return statement (and it confuses some compilers, IIRC).
> 
> Any compiler that confuses should be recycled into random bits.  :-)
> Seriously, that's simply fundamental correctness; we shouldn't use any
> compiler that fubars that (and I can't imagine any non-toy compiler does).

Yeah, compiler-friendliness wasn't my primary concern -- it's more of a style & clarity thing.  (It's been mentioned to me multiple times by other reviewers, and it's part of the JST Reviewer simulacrum, so I've internalized it as a general mozilla-coding-style thing.)

(I can imagine cases where else-after-return would be helpful readability-wise, but I don't think that was the case in the chunks that I flagged.)

Comment 85

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v5) - Implementation for nsImageFrame

Sending up a more polished version. Not spamming everyone with this yet, just getting a second look over by dholbert. If he gives the ok, then I will send it up for review to module owners.

Comment 86

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v4) - Implementation for RasterImage

Sending up a more polished version. Not spamming everyone with this yet, just getting a second look over by dholbert. If he gives the ok, then I will send it up for review to module owners.

Comment 87

[Daniel Holbert [:dholbert]]

Comment on attachment 548275 [details] [diff] [review]
Patch 1 (v5) - Implementation for nsImageFrame

Looks good! Just 2 nits:

>+nsImageRefreshObserver::Deregister()
>+{
[...]
>+  // mFrame is being destroyed, so we want to drop our reference to it so we
>+  // don't accidentally dereference a pointer that's been deleted. this will
>+  // only happen in the (very extreme) case where we somehow outlive the owning
>+  // nsImageFrame & get callbacks after it's been deleted.
>+  mFrame = nsnull;

s/extreme/unexpected/ or something. ("extreme" gives me the impression that it's something that *can* happen, by design -- when really, that's not the case as far as we know)

>+  // Handle to the refresh driver observation helper class
>+  // Refcounted because the nsRefreshDriver temporarily AddRef's this object
>+  // during each refresh ping. we don't want to use an nsAutoPtr because this
>+  // object will temporarily have multiple references to it, but it should
>+  // only be deleted when its nsImageFrame object is destroyed, not before.
>+  nsRefPtr<nsImageRefreshObserver> mRefreshObs;

capitalize "we don't want" (new sentence in middle of paragraph)

Comment 88

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v6) - Implementation for nsImageFrame

Made modifications as suggested by dholbert, submitting for final review.

Comment 89

[Scott Johnson (:jwir3)]

Comment on attachment 548284 [details] [diff] [review]
Patch 2 (v6) - Implementation for nsImageFrame

Requesting review for layout portion of this bug. Since Roc is on holiday, I'm also requesting review from dbaron.

Comment 90

[Daniel Holbert [:dholbert]]

Comment on attachment 548277 [details] [diff] [review]
Patch 2 (v4) - Implementation for RasterImage

>+bool
>+RasterImage::AdvanceFrame(nsIntRect* aDirtyRect, TimeStamp aTime)

>+  bool haveFullNextFrame = !mDecoder || nextFrameIndex
>+      < mDecoder->GetCompleteFrameCount();

Not enough indentation on second line there.

Also, best to avoid splitting a condition (nextFrameIndex < mDeco...) across multiple lines if you don't have to.  Maybe drop nextFrameIndex down to the next line?

>+  if (!(timeout > 0)) {
>+    mAnimationFinished = PR_TRUE;
>+    EvaluateAnimation();
>+  }

This should just change to "if (timeout <= 0)" (BUT maybe not exactly, read on....)

Also, this condition doesn't make sense to me.  It looks like this is checking for the "show this frame forever" condition, but IIRC (based on a comment elsewhere in the file) that's specifically for timeouts of -1.  So I'd think this should really be < instead of <=, so we actually want this:
  "if (timeout < 0) { //  -1 means show this frame forever"

Do you know if there was a reason for the existing behavior? (making 0-timeout trigger "animation finished" behavior)

>+    // Change frame and announce it

s/and announce it//  (This function no longer handles notification)

>+    if (NS_FAILED(DoComposite(&frameToUse, aDirtyRect, prevFrame,
>+            nextFrame, nextFrameIndex))) {

Fix indentation there.

>+      // something went wrong, move on to next
>+      NS_WARNING("RasterImage::AdvanceFrame(): Compositing of frame failed");
>+      nextFrame->SetCompositingFailed(PR_TRUE);
>+      mAnim->currentAnimationFrameIndex = nextFrameIndex;
>+      return false;

We probably want to set currentAnimationFrameTime there, too, right?  Otherwise that will be a stale/bogus value, the next time we hit RefreshRequested.

> RasterImage::RequestRefresh(const mozilla::TimeStamp& aTime)
> {
>-  // TODO: Implement me as part of b666446
>+  EnsureAnimExists();
>+  if (!mAnimating || !ShouldAnimate()) {
>+    return;
>+  }

As noted in in comment 75, I'm pretty sure we only want to call EnsureAnimExists() after we've determined that we're animating.  So, drop that EnsureAnimExists call down a few lines, to be after the early-return.

>+  // only advance the frame if the current time is greater than or
>+  // equal to the last frame time + the frame's offset

s/offset/timeout/ (this is the only place you use the term "offset")

>+  TimeStamp timePlusTimeout = currentFrameTime + durationOfTimeout;
[...]
>+  while (timePlusTimeout <= aTime) {
[...]
>+    timePlusTimeout = currentFrameTime + durationOfTimeout;

The significance of "timePlusTimeout" isn't immediately clear from its name.  Maybe rename to "frameEndTime" or "nextFrameStartTime" or something? That would make the loop condition a little easier to grock at a glance.

Also, RE the loop structure -- so it currently works like this:
 SetTimeVariablesForCurrentFrame;
 while (condition) {
   Advance Frame;
   SetTimeVariablesForCurrentFrame;
 }

That has some duplicated code (the "set time variables" stuff), and that creates possibility for bugs if we tweak something in the first duplicate chunk but not the second.

To avoid the duplicate code, could we instead structure it like this (or something equivalent/better):

 while (true) {
   SetABunchOfVariablesForCurrentFrame;
   if (!condition) {
     break;
   }
   Advance Frame;
 } 

>+  nsCOMPtr<imgIContainerObserver> observer(do_QueryReferent(mObserver));
>+
>+  while (timePlusTimeout <= aTime) {
>+    if (!observer) {
>+      NS_ERROR("Refreshing image after its imgRequest is gone");
>+      StopAnimation();
>+      return;
>+    }

Move the |observer| declaration & null-check down to where we actually use |observer|, inside the "if (frameAdvanced)" clause.  There's no need to null-check it on every loop iteration, and there's no need to create it at all for RefreshRequested() calls that don't trigger notification.

>+    frameAdvanced = frameAdvanced || this->AdvanceFrame(&dirtyRect, aTime);

s/this->//

>@@ -1117,53 +1244,46 @@ RasterImage::StartAnimation()
[...]
>+  // Default timeout to 100
>   PRInt32 timeout = 100;
[...]
>   if (currentFrame) {
>     timeout = currentFrame->GetTimeout();
>     if (timeout < 0) { // -1 means display this frame forever

We can ditch the variable |timeout| and the magic number 100 now -- looks like now we only care whether it's -1 or not. Replace with this:
      if (currentFrame->GetTimeout() < 0) { // -1 means show this frame forever

>+    // we need to set the time that this first frame was displayed
>+    // this is important for the new refresh driver-based animation
>+    // timing
>+    mAnim->currentAnimationFrameTime = TimeStamp::Now();

This multi-sentence comment is hard to parse without any capitalization/periods - add either or both of those features. :)

>+   * @param aDirtyRect a pointed to an nsIntRect which encapsulates the area to
>+   *        be repainted after the frame is advanced.

Looks like an extra "an" snuck into that comment.

Also, put [outparam] after "aDirtyRect" (see e.g. nsStyleAnimation.h for sample syntax)
Also, the general mozilla style is to make outparams be the final argument(s).  Could you switch the argument-order so that aDirtyRect is last?

>+   * @returns true, if the frame was successfully advanced, false if it was not
>+   *          able to be advanced (e.g. the frame to which we want to advance is
>+   *          still decoding).

It'd be good to also mention somewhere that if we return false, aDirtyRect will be untouched. (right?)

r=me with the above.

Comment 91

[Daniel Holbert [:dholbert]]

(In reply to comment #90)
> >+  bool haveFullNextFrame = !mDecoder || nextFrameIndex
> >+      < mDecoder->GetCompleteFrameCount();
> 
> Not enough indentation on second line there.

(er sorry, that's technically too _much_ indentation, by 2 spaces - I was imagining a layer of parenthesis that weren't there. Still would be best to have nextFrameIndex on same line as its "<".)

Comment 92

[Scott Johnson (:jwir3)]

> Do you know if there was a reason for the existing behavior? (making 0-timeout > trigger "animation finished" behavior)

No, I am not sure why this is. Someone else might be able to enlighten us further, but I think that in RasterImage::StartAnimation() we only check to see if it's > 0, so perhaps this is what we should do here as well.

Comment 93

[Scott Johnson (:jwir3)]

Whoops, hit enter too soon. Anyway, what I meant to add was that I will submit another bug to clarify/fix that issue.

Comment 94

[Joe Drew (not getting mail)]

(In reply to comment #91)
> (In reply to comment #90)
> > >+  bool haveFullNextFrame = !mDecoder || nextFrameIndex
> > >+      < mDecoder->GetCompleteFrameCount();
> > 
> > Not enough indentation on second line there.
> 
> (er sorry, that's technically too _much_ indentation, by 2 spaces - I was
> imagining a layer of parenthesis that weren't there. Still would be best to
> have nextFrameIndex on same line as its "<".)

Imagelib standard (aka, "My personal preference" :) ) is to vertically align either with the start of the bracket, the conditional clause, or failing all of that the beginning of the value, i.e.

bool foo = !bar && something ||
           somethingElse;

Comment 95

[Joe Drew (not getting mail)]

(In reply to comment #92)
> > Do you know if there was a reason for the existing behavior? (making 0-timeout > trigger "animation finished" behavior)
> 
> No, I am not sure why this is. Someone else might be able to enlighten us
> further, but I think that in RasterImage::StartAnimation() we only check to
> see if it's > 0, so perhaps this is what we should do here as well.

This code was introduced as "timeout >= 0" in CVS revision 1.5 of the file:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/modules/libpr0n/src&command=DIFF_FRAMESET&file=imgContainer.cpp&rev2=1.5&rev1=1.4

and then modified in revision 1.8 to be "> 0":

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/modules/libpr0n/src&command=DIFF_FRAMESET&file=imgContainer.cpp&rev2=1.8&rev1=1.7

Unfortunately neither of these revisions were linked to bugs, and my bugzilla searching has not come up with anything, which makes me think it was a sort of over-the-shoulder development that was common in the project at the time.

Comment 96

[Randell Jesup [:jesup] (needinfo me)]

I can add some historical context.

Animated GIFs were animating too fast in some cases, and IE (and maybe NS4) had put a minimum on the timeouts for animgifs.  That was the purpose of the 1.8 checkin. 

I *suspect* the reason for changing how 0 is handled was to a) avoid it sucking 100% of the CPU if there wasn't a a lower limit and it repeats, and b) if an anim is a run-once anim, the effect of a 0 timeout was to jump to the end.  But that's fuzzy.

I have a fuzzy memory of the discussions around this change; I was involved.  Not sure if they were in newsgroups or a bug.  Look for bugs closed by pav or hyatt in that timeframe.

Comment 97

[Randell Jesup [:jesup] (needinfo me)]

See bug 125137 for a related issue; it notes that IE of the era had a minimum anim timeout of 100ms.

After much searching I haven't found a bug associated with hyatt's checkin; perhaps searching on the sr= in the text will find it.

Comment 98

[Scott Johnson (:jwir3)]

Added bug 674329 to track this issue further.

Comment 99

[Scott Johnson (:jwir3)]

Sorry, meant bug 674239. Apparently a bit dyslexic today. ;)

Comment 100

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v5) - Implementation for RasterImage [r=dholbert]

r+ dholbert, submitting for final review with JOEDREW!. Please note that the issue with the timeout has not been fixed, as it was spun off into a different bug.

Comment 101

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v6) - Implementation for RasterImage [r=dholbert]

dholbert found a subtle bug in an edge case for the GetCurrentImgFrameEndTime() function I wrote. It has been fixed to return the max TimeStamp in the event of a negative timeout value. I added a macro that will probably be removed from RasterImage.h once prtypes.h is updated for bug 674277.

Comment 102

[Scott Johnson (:jwir3)]

Comment on attachment 548524 [details] [diff] [review]
Patch 2 (v6) - Implementation for RasterImage [r=dholbert]

Cancelling review because I found another bug in it. :)

Comment 103

[Scott Johnson (:jwir3)]

I cancelled the review because I found a problem with non-looping GIF images in RasterImage::GetCurrentImgFrameEndTime(). I'm fixing this problem right now, so you can feel free to review the rest, and I'll post an updated patch as soon as I fix the issue.

Comment 104

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v7) - Implementation for RasterImage [r=dholbert]

Fixed the infinite loop as described in the previous revision of this patch.

Comment 105

[Scott Johnson (:jwir3)]

dholbert mentioned that he had a concern about the GetCurrentImgFrameEndTime() implementation, because he was worried that it might not respond correctly if the value of timeout was negative. 

I implemented a work-around to this where GetCurrentImgFrameEndTime() returns a time equivalent to positive infinity when this happens, but I wasn't sure exactly how to test it, so I looked through the code to determine where this is set & how it is used. It looks like the GIF spec says that the delay time between frames is unsigned, and imgFrame::GetTimeout() returns 100 if the timeout that is passed in is anywhere between 0 and 10. Thus, it looks like it can't return a negative value.

dholbert said it warranted more investigation, as he didn't know how the RasterImage would know to stay forever on the last frame in the event of a non-looping animation. I investigated it and found that mLoopCount within the RasterImage object is decremented for each loop through the image. The number of loops to run is specified by the GIF decoder, or is -1 if it should loop forever. This is checked in RasterImage::AdvanceFrame() (was previously called Notify()). Thus, if it's currently at 0, and we're advancing to the last frame, the animation terminates, and the last frame is shown forever after that. 

So, now the question remains - why are there checks at all for timeout < 0? For example, in RasterImage::StartAnimation():

>  if (currentFrame) {
>    if (currentFrame->GetTimeout() < 0) {
>      // -1 means display this frame forever
>      mAnimationFinished = PR_TRUE;
>      return NS_ERROR_ABORT;
>    } 

It seems like the _only_ way that we would have a negative timeout is if someone explicitly called imgFrame::SetTimeout() with a negative value. So, I'm wondering if this is used in a non-gif animation (e.g. an APNG animation) or whether there is a risk of this timeout being negative. If not, I think it might make sense to remove these checks. My gut tells me that they were put in for a reason, though, so I'm wondering if anyone might know the reasoning behind having a possible negative value here, and even why imgFrame::GetTimeout returns a PRInt32 instead of a PRUint32.

Comment 106

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 548284 [details] [diff] [review]
Patch 2 (v6) - Implementation for nsImageFrame

Review of attachment 548284 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +216,5 @@
> +  mFrame = nsnull;
> +
> +  // sanity check to verify our reference is only held by one other class
> +  NS_ABORT_IF_FALSE(mRefCnt == 1,
> +                    "Multiple references exist to an image refresh observer");

I don't think we should do this. It's not fatal for something else to hang onto a reference longer than necessary. At best this should be an NS_WARNING.

@@ +311,5 @@
>    if (mDisplayingIcon)
>      gIconLoad->RemoveIconObserver(this);
>  
> +  // deregister our helper object with the refresh driver to complete tear-down
> +  mRefreshObs->Deregister();

Null check, this could be called after Init failed. If you don't want the null check, create the observer in the constructor instead of Init.

Hmm, it would be nice if we could only create and add the observer for animated images. Can we do that? Would save memory.

::: layout/generic/nsImageFrame.h
@@ +97,5 @@
> +/**
> + * A refresh observer for an nsImageFrame. It's necessary to have nsImageFrame
> + * respond to refresh driver events, but we don't want the refresh driver to
> + * AddRef/Release the nsImageFrame object, as this causes havoc with the frame
> + * management hierarchy. This class functions as a helper object that is ref-

The real reason is that frames aren't refcounted so AddRef/Release simply doesn't work.

@@ +109,5 @@
> +    nsImageRefreshObserver(nsImageFrame* aParent);
> +    ~nsImageRefreshObserver() {}
> +
> +    NS_IMETHOD_(nsrefcnt) AddRef(void);
> +    NS_IMETHOD_(nsrefcnt) Release(void);

Use NS_INLINE_DECL_REFCOUNTING

@@ +348,5 @@
> +  // Refcounted because the nsRefreshDriver temporarily AddRef's this object
> +  // during each refresh ping. We don't want to use an nsAutoPtr because this
> +  // object will temporarily have multiple references to it, but it should
> +  // only be deleted when its nsImageFrame object is destroyed, not before.
> +  nsRefPtr<nsImageRefreshObserver> mRefreshObs;

I think we can go for mRefreshObserver :-).

The comment is misleading. nsImageRefreshObserver is refcounted so nsAutoPtr is an absolute no-no, you can't use it on refcounted objects, period.

Comment 107

[Scott Johnson (:jwir3)]

Hi Roc:

Some responses to your comments on review -

> I don't think we should do this. It's not fatal for something else to hang 
> onto a reference longer than necessary. At best this should be an NS_WARNING.

Agreed. I changed it to NS_WARN_IF_FALSE.

> Null check, this could be called after Init failed. If you don't want the
> null check, create the observer in the constructor instead of Init.

I added a null check.

> Hmm, it would be nice if we could only create and add the observer for 
> animated images. Can we do that? Would save memory.

Sure, I think this makes sense. What's the best way to determine if an image is animated during construction? It seems like it only thinks it's animated if it has seen at least two frames. So, perhaps once the image recognizes there is more than one frame (e.g. StartAnimation()), then have it activate the refresh driver in the nsImageFrame (e.g. have StartAnimation() call mFrame->ActivateAnimation(), which would init the refresh observer)?
 
> Use NS_INLINE_DECL_REFCOUNTING

I originally did use this, but when I added this in place of the above code, I received the following compile errors:

error: conflicting return type specified for ‘virtual void nsImageRefreshObserver::AddRef()’
../../dist/include/nsRefreshDriver.h:71:60: error:   overriding ‘virtual nsrefcnt nsARefreshObserver::AddRef()’
../generic/nsImageFrame.h:110:966: error: conflicting return type specified for ‘virtual void nsImageRefreshObserver::Release()’
../../dist/include/nsRefreshDriver.h:72:60: error:   overriding ‘virtual nsrefcnt nsARefreshObserver::Release()’

So, I took the above code from an example implementation of nsARefreshObserver - NotificationController. I can make the appropriate change to NS_INLINE_DECL_REFCOUNTING, but I think I will need to modify nsARefreshObserver, which would in turn require numerous changes to child classes. (Unless I'm doing something incorrectly.) For reference, here's how I specified the macro:

    ~nsImageRefreshObserver() {}

    NS_INLINE_DECL_REFCOUNTING(nsARefreshObserver)

    void WillRefresh(mozilla::TimeStamp aTime);

> The real reason is that frames aren't refcounted so AddRef/Release simply doesn't work.

Made a change to the comment so it should be more accurate now.

> I think we can go for mRefreshObserver :-).

Yep, change made. :)

> The comment is misleading. nsImageRefreshObserver is refcounted so nsAutoPtr 
> is an absolute no-no, you can't use it on refcounted objects, period.

I made the change to the comment so it isn't so misleading.

Comment 108

[Daniel Holbert [:dholbert]]

(In reply to comment #107)
> > Hmm, it would be nice if we could only create and add the observer for 
> > animated images. Can we do that? Would save memory.
> 
> Sure, I think this makes sense. What's the best way to determine if an image
> is animated during construction? It seems like it only thinks it's animated
> if it has seen at least two frames. So, perhaps once the image recognizes
> there is more than one frame (e.g. StartAnimation()), then have it activate
> the refresh driver in the nsImageFrame

Alternately, we could have the nsImageFrame's imgIDecoderObserver (nsImageListener) listen for the second call to "OnStartFrame" or "OnStopFrame".  That tells us we've got 2 frames & hence we're animated.

Comment 109

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #107)
> Sure, I think this makes sense. What's the best way to determine if an image
> is animated during construction? It seems like it only thinks it's animated
> if it has seen at least two frames. So, perhaps once the image recognizes
> there is more than one frame (e.g. StartAnimation()), then have it activate
> the refresh driver in the nsImageFrame (e.g. have StartAnimation() call
> mFrame->ActivateAnimation(), which would init the refresh observer)?

That sounds good.

> > Use NS_INLINE_DECL_REFCOUNTING
> 
> I originally did use this, but when I added this in place of the above code,
> I received the following compile errors:
> 
> error: conflicting return type specified for ‘virtual void
> nsImageRefreshObserver::AddRef()’
> ../../dist/include/nsRefreshDriver.h:71:60: error:   overriding ‘virtual
> nsrefcnt nsARefreshObserver::AddRef()’
> ../generic/nsImageFrame.h:110:966: error: conflicting return type specified
> for ‘virtual void nsImageRefreshObserver::Release()’
> ../../dist/include/nsRefreshDriver.h:72:60: error:   overriding ‘virtual
> nsrefcnt nsARefreshObserver::Release()’

I think we can make nsARefreshObserver use NS_INLINE_DECL_REFCOUNTING without much trouble.

Comment 110

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #108)
> (In reply to comment #107)
> > > Hmm, it would be nice if we could only create and add the observer for 
> > > animated images. Can we do that? Would save memory.
> > 
> > Sure, I think this makes sense. What's the best way to determine if an image
> > is animated during construction? It seems like it only thinks it's animated
> > if it has seen at least two frames. So, perhaps once the image recognizes
> > there is more than one frame (e.g. StartAnimation()), then have it activate
> > the refresh driver in the nsImageFrame
> 
> Alternately, we could have the nsImageFrame's imgIDecoderObserver
> (nsImageListener) listen for the second call to "OnStartFrame" or
> "OnStopFrame".  That tells us we've got 2 frames & hence we're animated.

Do those calls happen in the absence of any WillRefresh calls? If so, that sounds good too.

Comment 111

[Scott Johnson (:jwir3)]

roc, dholbert, and I came up with a plan to add a list of imgIRequests to the nsRefreshDriver, instead of using the nsARefreshObserver child classes as I am doing in the current patch. This eliminates the need for a large number of these child classes, most with very similar functionality, and saves on memoery, as we don't have to allocate memory for these helper objects. As part of this, though, there needs to be some way to register these imgIRequests with the refresh driver. Most likely this will be through PresShell, which will need a new set of functions to add/remove these requests from the nsRefreshDriver. Now, given that these imgIRequests that are registering with the nsRefreshDriver really are observers, would it make more sense to add AddRefreshObserver(imgIRequest* aRequest) or AddImageRequest(imgIRequest* aRequest) to the nsPresContext class?

Comment 112

[Daniel Holbert [:dholbert]]

(In reply to comment #111)
> Most likely this will be through PresShell, which will need a new set of 
> functions to add/remove these requests from the nsRefreshDriver.
> Now, given that these imgIRequests that
> are registering with the nsRefreshDriver really are observers, would it make
> more sense to add AddRefreshObserver(imgIRequest* aRequest) or
> AddImageRequest(imgIRequest* aRequest) to the nsPresContext class?

I think you'll want to add the methods directly to the RefreshDriver, not the PresContext or PresShell.  (Note that anyone who has a handle on the PresContext or PresShell can ask for the RefreshDriver through them, and then communicate directly with it.)

I don't have strong feelings about the method-name. I've been imagining that it'd be something image-specific like "AddImageRequest()", but I don't think it matters too much as long as it's sensible.

Comment 113

[Scott Johnson (:jwir3)]

(In reply to comment #112)
 
> I think you'll want to add the methods directly to the RefreshDriver, not
> the PresContext or PresShell.  (Note that anyone who has a handle on the
> PresContext or PresShell can ask for the RefreshDriver through them, and
> then communicate directly with it.)

I agree, but I was previously using the PresShell::AddRefreshObserver(nsARefreshObserver* aObserver). I wasn't sure whether I should expose the function in the PresShell API; if this were the case, then for consistency I was going to name the function 'AddRefreshObserver' (i.e. an overloaded function), and just use an imgIRequest* as a parameter.

I'm somewhat interested to know why the Add/RemoveRefreshObserver functions defined in PresShell are defined in such a way as to (optionally) allow overriding, with the Add/RemoveRefreshObserverExternal() versions. It seems that bug 563327 addresses this, but I don't see a place anywhere in our code where Add/RemoveRefreshObserverExternal() is actually defined...

> I don't have strong feelings about the method-name. I've been imagining that
> it'd be something image-specific like "AddImageRequest()", but I don't think
> it matters too much as long as it's sensible.

Since I think the correct approach for now is to access the nsRefreshDriver directly through PresShell::RefreshDriver(), I will name the method pair 'Add/RemoveImageRequest()', and we can modify it later if desired. I originally thought that since it's actually functioning as a refresh observer, that might be made clear by using the 'AddRefreshObserver' method name, but I can see where that would get confusing in a different way. What do others think?

Comment 114

[Scott Johnson (:jwir3)]

In the above comment, I actually meant PresContext::RefreshDriver(), not PresShell()::RefreshDriver().

Comment 115

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #113)
> I'm somewhat interested to know why the Add/RemoveRefreshObserver functions
> defined in PresShell are defined in such a way as to (optionally) allow
> overriding, with the Add/RemoveRefreshObserverExternal() versions. It seems
> that bug 563327 addresses this, but I don't see a place anywhere in our code
> where Add/RemoveRefreshObserverExternal() is actually defined...

They're defined in nsPresShell.cpp.

The External versions were created so that code not linked into the same library as layout could call them. However, now that we always use libxul, that is probably no longer an issue --- we don't want extensions to be calling these. So we should probably just remove them.

> Since I think the correct approach for now is to access the nsRefreshDriver
> directly through PresShell::RefreshDriver(), I will name the method pair
> 'Add/RemoveImageRequest()', and we can modify it later if desired.

Sounds good.

Comment 116

[Scott Johnson (:jwir3)]

Attachment: Patch 1 - Implementation for nsRefreshDriver

This is the implementation for the nsRefreshDriver class. It was modified as part of work done to minimize memory impact of this patch (see comment 111).

Comment 117

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v7) - Implementation for nsImageFrame

Reworked nsImageFrame so that it no longer requires an nsARefreshObserver to use the new GIF animation architecture.

Comment 118

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v8) - Implementation for RasterImage [r=dholbert]

Comment 119

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 552012 [details] [diff] [review]
Patch 2 (v7) - Implementation for nsImageFrame

Review of attachment 552012 [details] [diff] [review]:
-----------------------------------------------------------------

We should try to make this only add an observer for animated images.

::: layout/base/nsIPresShell.h
@@ +103,5 @@
>  struct nsIntPoint;
>  struct nsIntRect;
>  class nsRefreshDriver;
>  class nsARefreshObserver;
> +class imgIRequest;

Why did you need to add this here?

::: layout/generic/nsImageFrame.cpp
@@ +164,5 @@
>  {
>    return new (aPresShell) nsImageFrame(aContext);
>  }
>  
> +NS_IMPL_FRAMEARENA_HELPERS(nsImageFrame);

Seems unneeded/wrong.

Comment 120

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v8) - Implementation for nsImageFrame

Fixed issues found during review and modified the nsImageFrame to only register imgIRequest objects with the refresh driver if its an animation or during decoding.

Comment 121

[Scott Johnson (:jwir3)]

Attachment: Patch 0 (v6) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]

Simply rebased the patch to the tip of mozilla-central.

Comment 122

[Scott Johnson (:jwir3)]

Attachment: Patch 3 - Implementation for nsImageBoxFrame

Implementation for tab icon and favicon animations.

Comment 123

[Scott Johnson (:jwir3)]

Comment on attachment 552276 [details] [diff] [review]
Patch 2 (v8) - Implementation for nsImageFrame

I'm re-requesting review after fixing issues described during previous review. I also added some code that will remove the imgIRequest as an observer from the nsRefreshDriver once decoding has stopped and the image isn't an animation. The imgIRequest is re-added to the nsRefreshDriver in OnStartDecode again, as it's necessary for other work that is being built on top of this for the nsRefreshDriver to notify images while they are being decoded.

Comment 124

[Daniel Holbert [:dholbert]]

(In reply to Scott Johnson (:jwir3) from comment #122)
> Created attachment 552288 [details] [diff] [review]
> Patch 3 - Implementation for nsImageBoxFrame
> 
> Implementation for tab icon and favicon animations.

(Note: we're shifting away from allowing animated favicons -- see bug 111373 comment 49 & after -- but I imagine we'll still need this patch for the throbber animation during pageload.)

Comment 125

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v2) - Implementation for nsRefreshDriver

Changed from nsTArray<nsRefPtr<imgIRequest> > to nsTArray<nsCOMPtr<imgIRequest> > (see bug 676603).

Comment 126

[Scott Johnson (:jwir3)]

> (Note: we're shifting away from allowing animated favicons -- see bug 111373
> comment 49 & after -- but I imagine we'll still need this patch for the
> throbber animation during pageload.)

Yeah, actually that's where I noticed there was a problem. ;) I didn't really notice that when you load an animated gif directly, a smaller version of the gif appears as the favicon. But, I did notice that the progress indicator didn't move, which made me suspicious something was wrong. :) At any rate, the nsImageBoxFrame falls into the category of 'nsImageFrame-like' classes, so it needed to be fixed anyway.

Comment 127

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Two things:
1) I think we should make the imgIRequest table in the refresh driver be a hash-set. Right now if you have a large number of images removing all the imgIRequests could easily be O(N^2).
2) I think we should add an optimization to track in nsImageFrame whether its imgIRequest is in the refresh driver. If it isn't, we shouldn't try to remove it. This will be more efficient and stop warning spam.

Comment 128

[Scott Johnson (:jwir3)]

Attachment: Patch 0 (v7) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]

Changed commit message to include review notation and more descriptive comments.

Comment 129

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v3) - Implementation for nsRefreshDriver

Minor changes to commit message to add more descriptive commentary.

Comment 130

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v8) - Implementation for nsImageFrame

Modified according to roc's review comments and changed commit message to include more descriptive commentary.

Comment 131

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v2) - Implementation for nsImageBoxFrame

Modified commit message to be more informative.

Comment 132

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v9) Implementation for RasterImage [r=dholbert]

Modified commit message to indicate reviewer and be more informative.

Comment 133

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 553051 [details] [diff] [review]
Patch 1 (v3) - Implementation for nsRefreshDriver

Review of attachment 553051 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsRefreshDriver.cpp
@@ +188,5 @@
> +nsRefreshDriver::AddImageRequest(imgIRequest* aRequest)
> +{
> +  nsPtrHashKey<imgIRequest>* hashKey = mRequests.PutEntry(aRequest);
> +  if (!hashKey) {
> +    return PR_FALSE;

Can just write
  if (!mRequests.PutEntry(aRequest))
    return PR_FALSE;

@@ +200,5 @@
> +PRBool
> +nsRefreshDriver::RemoveImageRequest(imgIRequest* aRequest)
> +{
> +  if (!mRequests.GetEntry(aRequest)) {
> +    return PR_FALSE;

Why do this check? This doubles the time taken in this function. Just call RemoveEntry and return void from this function.

@@ +274,5 @@
>    return sum;
>  }
>  
> +PRUint32
> +nsRefreshDriver::RequestCount() const

ImageRequestCount

@@ +479,5 @@
> +    static_cast<ImageRequestParameters*> (aUserArg);
> +  mozilla::TimeStamp* mostRecentRefresh = parms->ts;
> +  bool* didRefresh = parms->didRefresh;
> +  imgIRequest* req = aEntry->GetKey();
> +  NS_WARN_IF_FALSE(req, "Unable to retrieve the image request");

Might as well be NS_ASSERTION since we'll crash if it's null

@@ +482,5 @@
> +  imgIRequest* req = aEntry->GetKey();
> +  NS_WARN_IF_FALSE(req, "Unable to retrieve the image request");
> +  nsCOMPtr<imgIContainer> image;
> +  req->GetImage(getter_AddRefs(image));
> +  if (image.get()) {

if (image) {

@@ +486,5 @@
> +  if (image.get()) {
> +    image->RequestRefresh(*mostRecentRefresh);
> +
> +    if (didRefresh) {
> +      *didRefresh = true;

So didRefresh always gets set to true if the image request table is non-empty? Then you might as well remove the boolean and just test that instead.

::: layout/base/nsRefreshDriver.h
@@ +276,5 @@
>    bool mTimerIsPrecise;
>  
>    // separate arrays for each flush type we support
>    ObserverArray mObservers[3];
> +  RequestArray mRequests;

Call it RequestTable, since it's not an array.

@@ +292,5 @@
> +
> +  // Helper struct for processing image requests
> +  struct ImageRequestParameters {
> +      mozilla::TimeStamp* ts;
> +      bool* didRefresh;

Instead of using pointers, just use values; copy the timestamp in here, and the boolean can live in here directly.

Comment 134

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 553052 [details] [diff] [review]
Patch 2 (v8) - Implementation for nsImageFrame

Review of attachment 553052 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +280,5 @@
>    // Set the animation mode here
>    if (currentRequest) {
> +
> +    // register the imgIRequest with the refresh driver
> +    presContext->RefreshDriver()->AddImageRequest(currentRequest);

Should probably only set mRequestRegistered if this returns true.

@@ +621,5 @@
> +{
> +  nsPresContext* presContext = PresContext();
> +  if (!(presContext->RefreshDriver()->AddImageRequest(aRequest))) {
> +    return NS_ERROR_FAILURE;
> +  }

Shouldn't you be setting mRequestRegistered here?

@@ +703,5 @@
> +                      "Unable to deregister imgIRequest "
> +                      "with a null presContext");
> +    nsCOMPtr<imgIRequest> currentRequest;
> +    aImageLoader->GetRequest(nsIImageLoadingContent::CURRENT_REQUEST,
> +                          getter_AddRefs(currentRequest));

Fix indent

Comment 135

[Scott Johnson (:jwir3)]

> Can just write
>   if (!mRequests.PutEntry(aRequest))
>     return PR_FALSE;

Ok, made this change.

> Why do this check? This doubles the time taken in this function. Just call 
> RemoveEntry and return void from this function.

Modified to reflect this.

> ImageRequestCount

Made this change.

> Might as well be NS_ASSERTION since we'll crash if it's null

Agreed. I changed this as well.

> if (image) {

I originally had this this way, but I was finding sometimes that the smart pointer that was returned was nonnull, but that the actual mRawPtr within it was 0x0, causing it to get past this check, but still segfault. I thought that checking the actual raw pointer was hackish, but perhaps there is another method for verifying that GetImage() returns a valid raw pointer?

> So didRefresh always gets set to true if the image request table is 
> non-empty? Then you might as well remove the boolean and just test that 
> instead.

Well, didRefresh is supposed to be true if at least one of the imgIRequests subscribed for notification successfully performed a refresh. This will likely amount to the table being non-empty, but I didn't know if it was possible that perhaps there might be an image request that was subscribed for notification, but for whatever reason, we failed in the GetImage() call, thus resulting in the RequestRefresh() method not being invoked.

I could probably simplify this, if you think we can ignore this strange corner case.

> Call it RequestTable, since it's not an array.
Done.

> Instead of using pointers, just use values; copy the timestamp in here, and 
> the boolean can live in here directly.
Sounds good. I'll make that change.

Comment 136

[Scott Johnson (:jwir3)]

> Should probably only set mRequestRegistered if this returns true.
Added an if statement to check for this.

> Shouldn't you be setting mRequestRegistered here?
Yes, thanks for the catch.

> Fix indent
Done.

Comment 137

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v9) - Implementation for nsImageFrame

More review findings fixed.

Comment 138

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v3) - Implementation for nsImageBoxFrame [r=roc]

Minor fix to propagate some changes made to patch 2.

Comment 139

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Scott Johnson (:jwir3) from comment #135)
> > if (image) {
> 
> I originally had this this way, but I was finding sometimes that the smart
> pointer that was returned was nonnull, but that the actual mRawPtr within it
> was 0x0, causing it to get past this check, but still segfault.

I don't know what you mean by this. I believe "if (image)" does an implicit coercion to T*, effectively the same as a get().

> > So didRefresh always gets set to true if the image request table is 
> > non-empty? Then you might as well remove the boolean and just test that 
> > instead.
> 
> Well, didRefresh is supposed to be true if at least one of the imgIRequests
> subscribed for notification successfully performed a refresh. This will
> likely amount to the table being non-empty, but I didn't know if it was
> possible that perhaps there might be an image request that was subscribed
> for notification, but for whatever reason, we failed in the GetImage() call,
> thus resulting in the RequestRefresh() method not being invoked.

GetImage should never fail. Maybe it does in exceptional circumstances (OOM or something) but it's OK to do an unnecessary refresh in such cases.

Comment 140

[Scott Johnson (:jwir3)]

> I don't know what you mean by this. I believe "if (image)" does an implicit 
> coercion to T*, effectively the same as a get().

Hm... yes, I think I was mistaken about the location I previously mentioned that there was a segfault.

> GetImage should never fail. Maybe it does in exceptional circumstances (OOM or 
> something) but it's OK to do an unnecessary refresh in such cases.

Ok, I'll make that change then.

Comment 141

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v10) - Implementation for nsImageFrame

Shifted a couple of things around that belonged in a different patch.

Comment 142

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v4) - Implementation for nsRefreshDriver

Shifted a couple of things around that belonged in a different patch.

Comment 143

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 553107 [details] [diff] [review]
Patch 1 (v4) - Implementation for nsRefreshDriver

Review of attachment 553107 [details] [diff] [review]:
-----------------------------------------------------------------

I still don't think didRefresh is needed. We can just check if mRequests is empty.

Comment 144

[Scott Johnson (:jwir3)]

> I still don't think didRefresh is needed. We can just check if mRequests is 
> empty.

Agreed, I removed it from this latest patch.

Comment 145

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v5) - Implementation for nsRefreshDriver

Modifications to overcome a few review comments.
Also, I changed the hash key type to nsISupportsHashKey, as I was getting Segfaults without this where the parent container of an imgIRequest (the nsISupports pointer in the vtable) was becoming messed up. This seems to have fixed that issue.

Comment 146

[Scott Johnson (:jwir3)]

Attachment: Patch 4 Implementation for nsBulletFrame

Implementation for the next of the 'nsImageFrame-like' classes: nsBulletFrame. I have a (non-automated test) I will post if you want to run it, as well.

Comment 147

[Scott Johnson (:jwir3)]

Attachment: Test Case 2 - Animated Bullets

A test case for nsBulletFrame.

Comment 148

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 553350 [details] [diff] [review]
Patch 4 Implementation for nsBulletFrame

Review of attachment 553350 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK but I think we should do CSS background images first and then figure out how to share as much code as possible.

Comment 149

[Joe Drew (not getting mail)]

Comment on attachment 553058 [details] [diff] [review]
Patch 5 (v9) Implementation for RasterImage [r=dholbert]

Review of attachment 553058 [details] [diff] [review]:
-----------------------------------------------------------------

r=me as long as you add tests and correct the below.

::: modules/libpr0n/src/RasterImage.cpp
@@ +324,5 @@
> +  // Figure out if we have the next full frame. This is more complicated than
> +  // just checking for mFrames.Length() because decoders append their frames
> +  // before they're filled in.
> +  NS_ABORT_IF_FALSE(mDecoder || nextFrameIndex <= mFrames.Length(),
> +      "How did we get 2 indices too far by incrementing?");

Correctly indent this second line.

@@ +425,5 @@
> +  nsIntRect dirtyRect;
> +
> +  while (currentFrameEndTime <= aTime) {
> +    TimeStamp oldFrameEndTime = currentFrameEndTime;
> +    frameAdvanced = frameAdvanced || AdvanceFrame(aTime, &dirtyRect);

Am I misreading this, or will this short-circuit and never advance past the first frame?

(Either way, you should add a test to make sure we can advance multiple frames via the refresh driver.)

@@ +1308,5 @@
>      }
> +
> +    // We need to set the time that this initial frame was first displayed.
> +    // This is important for the new refresh driver-based animation
> +    // timing.

No need to call it "new" here, because time-based references in code always get hilariously out of date. :)

Just say the first line in this sentence, and maybe explain that it's used in AdvanceFrame.

Comment 150

[Scott Johnson (:jwir3)]

> as long as you add tests
Definitely. I have some tests that I am going to add as separate patches. I haven't posted them yet, because they aren't working completely yet. Once they are working, I will post these. They include a basic animated gif mochitest, a crash test for multi-frame animated gifs, and a test for bullet animations. I will likely have others, but I haven't finished them yet. 

> Correctly indent this second line.
Will do.

> Am I misreading this, or will this short-circuit and never advance past the 
> first frame?
Hm, yes, I think you're correct. If the frameAdvanced variable is true, it won't actually call AdvanceFrame(). I'll fix this so that it calls AdvanceFrame() in succession, regardless of whether it previously advanced a frame.

> (Either way, you should add a test to make sure we can advance multiple 
> frames via the refresh driver.)
Sure, I'll add a test for this as soon as I finish up changing the nsImageFrame-like classes.

> No need to call it "new" here, because time-based references in code always 
> get hilariously out of date. :)
Sure. I'll remove this time-based comment. I started off calling it 'new' for a while, but realized this would get out of date very quickly, so haven't done that in the newest patches I posted.

Comment 151

[Scott Johnson (:jwir3)]

Attachment: Patch 9 (v9) - Implementation for RasterImage [r=dholbert,JOEDREW!]

Implementation of RasterImage with JOEDREW!'s review findings addressed.

Comment 152

[Scott Johnson (:jwir3)]

Attachment: Patch 7 - Implementation for nsImageLoader

Implementation of nsImageLoader (background images) using refresh driver.

Comment 153

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v2) - Implementation for nsBulletFrame

Coalesced a bunch of the duplicate code from other frames to utilize the refresh driver into nsLayoutUtils and added calls to these functions.

Comment 154

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v5) - Implementation for nsImageBoxFrame [r=roc]

Coalesced a bunch of the duplicate code from other frames to utilize the refresh driver into nsLayoutUtils and added calls to these functions.

Comment 155

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v9) - Implementation for nsImageFrame [r=roc]

Coalesced a bunch of the duplicate code from other frames to utilize the refresh driver into nsLayoutUtils and added calls to these functions.

Comment 156

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v9) - Implementation for nsImageFrame [r=roc]

Added reviewer note in commit message.

Comment 157

[Scott Johnson (:jwir3)]

Attachment: Patch 3 - Implementation for nsLayoutUtils

Implementation of nsLayoutUtils' coalesced code to avoid duplication in multiple frames.

Comment 158

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v3) - Implementation for nsRefreshDriver [r=roc]

Comment 159

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v7) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]

No changes, just reposting with a different patch # to make things consistent and clear which patches to apply first.

Comment 160

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 553665 [details] [diff] [review]
Patch 3 - Implementation for nsLayoutUtils

Review of attachment 553665 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with those fixed

::: layout/base/nsLayoutUtils.cpp
@@ +4338,5 @@
> +                                      bool* aRegistered)
> +{
> +  // Deregister our imgIRequest with the refresh driver to
> +  // complete tear-down, but only if it has been registered
> +  if (*aRegistered) {

Less indentation to do an early exit:
  if (!*aRegistered || !aFrame)
    return;

@@ +4346,5 @@
> +                        "Unable to deregister imgIRequest "
> +                        "with a null presContext");
> +      presContext->RefreshDriver()->RemoveImageRequest(aRequest);
> +
> +      if (aRegistered) {

Don't test aRegistered, it should never be null (and if it was, you'd have crashed already)

::: layout/base/nsLayoutUtils.h
@@ +1436,5 @@
> +   *        register with the refresh driver of the frame.
> +   * @param aRegistered A pointer to a boolean value indicating whether this
> +   *        imgIRequest object has already been registered with the refresh
> +   *        driver for this frame. It is the responsibility of the caller to
> +   *        maintain this boolean value, but the function will utilize it

It's not really the caller's responsibility. RegisterImageRequest will set it. Better to say that this value tracks whether the image is registered, and so of course it must initially be false, and these three APIs all use and maintain the value. Fix all three versions of this comment.

Comment 161

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 553663 [details] [diff] [review]
Patch 4 (v9) - Implementation for nsImageFrame [r=roc]

Review of attachment 553663 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.h
@@ +265,5 @@
> +   *
> +   * @param aImageLoader An nsIImageLoadingContent object used to retrieve the
> +   *        current imgIRequest.
> +   */
> +  void DeregisterFromRefreshDriver(nsCOMPtr<nsIImageLoadingContent> aImageLoader);

This should go away now?

Comment 162

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v10) - Implementation for nsImageFrame [r=roc]

> This should go away now?
Yep. Fixed in this version of the patch.

Comment 163

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v2) - Implementation for nsLayoutUtils [r=roc]

> Less indentation to do an early exit:
>   if (!*aRegistered || !aFrame)
>     return;
Fixed.

> Don't test aRegistered, it should never be null (and if it was, you'd have 
> crashed already)
Ok. Fixed.

> It's not really the caller's responsibility. RegisterImageRequest will set 
> it. Better to say that this value tracks whether the image is registered, and 
> so of course it must initially be false, and these three APIs all use and 
> maintain the value. Fix all three versions of this comment.
Reworded to make this a bit more clear.

Comment 164

[Scott Johnson (:jwir3)]

Attachment: Patch 8 - Implementation for nsSVGImageFrame

Implementation for animated GIFs referenced within <image> elements embedded in svg documents (ok, so that was a mouthful).

Comment 165

[Scott Johnson (:jwir3)]

Attachment: Mochitest 1 - Basic Test for Functionality of Animated GIF Images

This is a test to verify that animated gif images work as expected. It loads two images (reference frames), and a two-frame animated gif. It currently only checks to verify that the last frame matches, but I've included a second frame so that in the future it will be easier to extend to check each frame individually.

Comment 166

[Scott Johnson (:jwir3)]

Attachment: Patch 11 (v10) - Implementation for RasterImage [r=dholbert,JOEDREW!]

Fixed a bug found by attachment 554990 [details] [diff] [review] where a non-looping animated gif would reset to frame 0 after it had completed.

Comment 167

[Scott Johnson (:jwir3)]

Attachment: Patch 9 - Implementation for xul tree

This is the implementation for the XUL tree image listener (mostly nsTreeBodyFrame and nsTreeImageListener). Just to jump ahead of an issue likely to come up in review, you'll see that I removed the inclusion of nsTreeImageListener.h from nsTreeBodyFrame.h, and added it to nsTreeBodyFrame.cpp, replacing the occurrence in the .h file with a forward declaration. This is so that I can include nsTreeBodyFrame.h in nsTreeImageListener.h, as I was getting a forward declaration compile error with just the class defined at the top of the file. Including the entire .h file actually helped resolve this issue, and I'm just being explicit about this in case you have questions about that stuff.

Comment 168

[Scott Johnson (:jwir3)]

Attachment: Patch 10 - Implementation of nsSVGFEImageElement

This should be the last layout component that needs to change with this patch, the SVG filter image. I will shortly be uploading tests for as many of these components as I am able to write reasonable tests for.

Comment 169

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 555439 [details] [diff] [review]
Patch 9 - Implementation for xul tree

Review of attachment 555439 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
@@ +119,5 @@
> +  // because we might have multiple imgIRequests registered from the frame,
> +  // and we don't want to mark all of the imgIRequests as deregistered until
> +  // they actually are.
> +  bool requestRegistered = true;
> +  nsTreeBodyFrame* frame = static_cast<nsTreeBodyFrame*> (aData);

No space between > and (

@@ +4485,5 @@
>    }
>  }
>  
> +void
> +nsTreeBodyFrame::ClearCreatedListeners()

Call this DetachImageListeners, it's clearer

@@ +4670,5 @@
> +
> +nsresult
> +nsTreeBodyFrame::OnStartDecode(imgIRequest* aRequest)
> +{
> +  nsLayoutUtils::RegisterImageRequest(this, aRequest, &mRequestRegistered);

If there are multiple requests at a time (which there are, right?) then this won't work because for the second and subsequent requests mRequestRegistered will be true already.

It seems to me we should just not have an mRequestRegistered on nsTreeBodyFrame.

Probably instead of having a dummy variable we should let the bool pointer be null, and if it's null, then we'd just make pessimistic assumptions in those helper methods.

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.h
@@ +617,5 @@
>    PRPackedBool mReflowCallbackPosted;
> +
> +  // Array to keep track of which listeners we created and thus
> +  // have pointers to us.
> +  nsTArray<nsTreeImageListener*> mCreatedListeners;

Nothing keeps these alive, it seems like you'll have dangling pointers here if the listener is released and goes away?

I think the listeners' destructor should call back to the frame to remove the listener from the array.

Also, if we have a lot of listeners, removing from the array might get expensive, so we probably should use a hash-set here.

::: layout/xul/base/src/tree/src/nsTreeImageListener.h
@@ +97,5 @@
> +  /**
> +   * Clear the internal frame pointer to prevent dereferencing an object
> +   * that no longer exists.
> +   */
> +  void ClearFrame();

This can just be declared inline here.

Comment 170

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 555496 [details] [diff] [review]
Patch 10 - Implementation of nsSVGFEImageElement

Review of attachment 555496 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGFilters.cpp
@@ +5420,5 @@
>    AddStatesSilently(NS_EVENT_STATE_BROKEN);
> +
> +  // Register our image request with the refresh driver.
> +  nsLayoutUtils::RegisterImageRequest(GetPrimaryFrame(), mCurrentRequest,
> +                                      &mRequestRegistered);

Is mCurrentRequest ever non-null here? If so, how?

@@ +5428,5 @@
>  {
> +  if (mCurrentRequest) {
> +    nsIFrame *frame = GetPrimaryFrame();
> +    nsLayoutUtils::DeregisterImageRequest(frame, mCurrentRequest,
> +                                          &mRequestRegistered);

"frame" could be null here. For example, the element could be display:none.

@@ +5652,5 @@
> +nsSVGFEImageElement::OnStartDecode(imgIRequest *aRequest)
> +{
> +  // Register with the refresh driver.
> +  nsIFrame *frame = GetPrimaryFrame();
> +  nsLayoutUtils::RegisterImageRequest(frame, aRequest, &mRequestRegistered);

"frame" could be null here.

@@ +5665,5 @@
>  {
> +  // If we're not animated, then we want to deregister from the refresh driver.
> +  nsIFrame *frame = GetPrimaryFrame();
> +  nsLayoutUtils::DeregisterImageRequestIfNotAnimated(frame, aRequest,
> +                                                     &mRequestRegistered);

"frame" could be null here too.

Comment 171

[Scott Johnson (:jwir3)]

Attachment: Patch 9 (v2) - Implementation for xul tree

> No space between > and (
Fixed.

> Call this DetachImageListeners, it's clearer
Fixed.

> If there are multiple requests at a time (which there are, right?) then this won't work because for the second and subsequent
> requests mRequestRegistered will be true already.
> It seems to me we should just not have an mRequestRegistered on nsTreeBodyFrame.
> Probably instead of having a dummy variable we should let the bool pointer be null, and if it's null, then we'd just make 
> pessimistic assumptions in those helper methods.

True. I missed this occurrence of the issue, but it's been fixed now as you suggested. The handling of the null boolean parameter is included in the patch I will post next, in nsLayoutUtils.

> Nothing keeps these alive, it seems like you'll have dangling pointers here if the listener is released and goes away?
> I think the listeners' destructor should call back to the frame to remove the listener from the array.
Done.

> Also, if we have a lot of listeners, removing from the array might get expensive, so we probably should use a hash-set here.
I implemented this. Because of the inheritance structure of nsTreeImageListener, it took me a long time to figure out exactly how this should be done. khuey thinks we should remove the nsITreeImageListener altogether (see bug 682077), but I'm not sure this is possible because of the way we query the interface here: http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2146

I had to separate out this interface into a separate header file, though because of circular problems. It breaks blame, unfortunately, but I didn't know how else to handle this.

> This can just be declared inline here.
I originally did this, but then I had to pull it out into the interface in order to utilize QueryInterface() on the nsITreeImageListener but then actually utilize this function in DetachImageListener() (the hashtable's enumerator function), so it's no longer declared inline, but there is a reason for it now. :)

Comment 172

[Scott Johnson (:jwir3)]

> "frame" could be null here. For example, the element could be display:none.
> "frame" could be null here.
> "frame" could be null here too.

I'm not exactly sure what you mean about these comments... I see what you are saying about frame possibly being null, but it's ok even if it is null, because Register/Deregister in nsLayoutUtils checks to make sure the frame passed in is non-null before doing anything - if the frame passed in is null, then it simply returns without performing any action.

Comment 173

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v3) - Implementation for nsLayoutUtils [r=roc]

Just re-submitting for review so you can verify that I handled the situation where the boolean pointer argument might be null.

Comment 174

[Scott Johnson (:jwir3)]

> Is mCurrentRequest ever non-null here? If so, how?

No, I don't think it is. The odd thing is that if I remove that statement from the constructor, the animation never runs (i.e. it simply stays on the first frame). But, if I add that call into the constructor, from what I can tell in gdb, it's not doing anything (just returning early), but the animation runs. 

I'll have to look into this further.

Comment 175

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Scott Johnson (:jwir3) from comment #172)
> > "frame" could be null here. For example, the element could be display:none.
> > "frame" could be null here.
> > "frame" could be null here too.
> 
> I'm not exactly sure what you mean about these comments... I see what you
> are saying about frame possibly being null, but it's ok even if it is null,
> because Register/Deregister in nsLayoutUtils checks to make sure the frame
> passed in is non-null before doing anything - if the frame passed in is
> null, then it simply returns without performing any action.

OK, then what happens if the image starts loading while the element has a frame, but then while the image is loading it gets made display:none and the frame goes away? It seems the request will never be unregistered.

Comment 176

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 555863 [details] [diff] [review]
Patch 3 (v3) - Implementation for nsLayoutUtils [r=roc]

Review of attachment 555863 [details] [diff] [review]:
-----------------------------------------------------------------

The code here is fine but it has the potential for misuse like I mentioned in my previous comment. Maybe it's better to not handle null frames here so the caller knows it has to clean up if a frame goes away.

Comment 177

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(The handling of null aRegistered is fine.)

Comment 178

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 555860 [details] [diff] [review]
Patch 9 (v2) - Implementation for xul tree

Review of attachment 555860 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.h
@@ +627,5 @@
>    PRPackedBool mReflowCallbackPosted;
> +
> +  // Hash table to keep track of which listeners we created and thus
> +  // have pointers to us.
> +  nsTHashtable<nsISupportsHashKey> mCreatedListeners;

I think you can just use nsTHashtable<nsPtrHashKey<nsTreeImageListener>> here. You don't need to hold a strong reference since any listener in the table will remove itself via RemoveTreeImageListener before it dies.

In fact, I think you have a quasi-leak right now, at least as long as the frame stays alive. This hashtable keeps the listeners alive. The listeners won't remove themselves because they only remove themselves in their destructor, which won't be called as long as the frame is keeping them alive!

Comment 179

[Scott Johnson (:jwir3)]

Attachment: Patch 10 (v2) - Implementation of nsSVGFEImageElement

> Is mCurrentRequest ever non-null here? If so, how?
Posting new patch that removes this dead code.

Comment 180

[Scott Johnson (:jwir3)]

Attachment: Mochitest 2 - Test for functionality of nsSVGImageFrame component

Another mochitest for this bug. More tests are coming as soon as I get them finished.

Comment 181

[Scott Johnson (:jwir3)]

Attachment: Mochitest 1 - Basic Test for Functionality of Animated GIF Images

Changed the timeout to 2 minutes.

Comment 182

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Scott Johnson (:jwir3) from comment #179)
> Created attachment 556068 [details] [diff] [review]
> Patch 10 (v2) - Implementation of nsSVGFEImageElement
> 
> > Is mCurrentRequest ever non-null here? If so, how?
> Posting new patch that removes this dead code.

What about comment #175?

Comment 183

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 556123 [details] [diff] [review]
Mochitest 2 - Test for functionality of nsSVGImageFrame component

Review of attachment 556123 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with that.

::: modules/libpr0n/test/mochitest/Makefile.in
@@ +98,5 @@
>                  test_animation.html \
>                  animated-gif-frame1.gif \
>                  animated-gif-frame2.gif \
>                  animated-gif.gif \
> +				test_svg_animatedGIF.html \

Fix indent

::: modules/libpr0n/test/mochitest/test_svg_animatedGIF.html
@@ +102,5 @@
> +}
> +
> +function doMyOnStopFrame() {
> +  myOnStopFrame();
> +  setTimeout(doMyOnStopFrame, 1000);

I think we should do this every 10ms. No reason not to and it'll make the test finish faster.

Comment 184

[Scott Johnson (:jwir3)]

> What about comment #175?

I'm looking at that right now. It looks like all of the other functions in nsLayoutUtils that require a frame do an NS_PRECONDITION, which, iiuc, is the same as an NS_ASSERTION. MDN says not to use NS_ASSERTION, but instead use NS_ABORT_IF_FALSE. 

Should I go with the standard of the file and use NS_PRECONDITION, or use NS_ABORT_IF_FALSE if aFrame is null?

Comment 185

[Scott Johnson (:jwir3)]

Attachment: Case where the SVG image filter works fine

Comment 186

[Scott Johnson (:jwir3)]

Attachment: Case where the SVG image filter doesn't animate

Only doesn't work if viewed directly with all patches up to patch 11 applied.

Comment 187

[Scott Johnson (:jwir3)]

I just noticed that one of my test cases for SVG filters works fine (attachment 556141 [details]) when the SVG file is placed into the HTML document using the <embed> tag. If I view the svg file directly, though, the animation doesn't play (attachment 556142 [details]). It does play in the current nightly, so I'm wondering if this is a different class that I haven't actually modified yet.

Strangely enough, if I add that dead code back into the nsSVGFEImageElement patch (the call to RegisterImageRequest), it will work if I view the SVG file directly, even though it really shouldn't do anything. 

After running this through gdb, it looks like mAnimating isn't getting set correctly for the imgIRequest used in the nsSVGFEImageElement - but only sometimes. If I keep hitting f5 over and over, it will eventually load the animation and start playing.

Comment 188

[Scott Johnson (:jwir3)]

Attachment: Case where the SVG image filter works fine

Accidentally didn't use data uri with this attachment.

Comment 189

[Scott Johnson (:jwir3)]

dholbert has been assisting me with the problem I describe in comment 187. We believe this to be caused by a race condition. It seems that in ShouldAnimate(), the number of animation consumers is not getting appropriately incremented. ShouldAnimate() is returning false due to mAnimationConsumers being 0. 

However, imgRequestProxy::IncrementAnimationConsumers() is getting called for the imgIRequest representing the animated image from nsDocument::AddImage. Stepping through it with gdb causes the image to being animating, which dholbert and I think is probably more evidence that it's a race condition. 

I will keep investigating it and hopefully have an answer early next week.

Comment 190

[Daniel Holbert [:dholbert]]

Attachment: Case where the SVG image filter doesn't animate (with 1-second delay)

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #170)
> @@ +5652,5 @@
> > +nsSVGFEImageElement::OnStartDecode(imgIRequest *aRequest)
> > +{
> > +  // Register with the refresh driver.
> > +  nsIFrame *frame = GetPrimaryFrame();
> > +  nsLayoutUtils::RegisterImageRequest(frame, aRequest, &mRequestRegistered);
> 
> "frame" could be null here.

From a bit of debugging, I believe Comment 187 - 189 are due to this chunk that roc highlighted -- we don't have a frame yet, at the point where we're trying to register for callbacks from the refresh driver.  (and no frame --> no registration)  So there's basically a race condition between frame creation & OnStartDecode being called.  (and if OnStartDecode wins the race, we never get registered for refresh callbacks)

While jwir3's existing testcase sometimes loads correctly (e.g. with <embed> per comment 187) & sometimes fails, here's a variant with an initial "display:none" for 1 second, which delays frame-creation and makes us always lose the race (even in a testcase with <embed>)

Comment 191

[Scott Johnson (:jwir3)]

(In reply to Daniel Holbert [:dholbert] from comment #190)

> From a bit of debugging, I believe Comment 187 - 189 are due to this chunk
> that roc highlighted -- we don't have a frame yet, at the point where we're
> trying to register for callbacks from the refresh driver.  (and no frame -->
> no registration)  So there's basically a race condition between frame
> creation & OnStartDecode being called.  (and if OnStartDecode wins the race,
> we never get registered for refresh callbacks)

Thanks for assisting me with this. I will fix these functions. 
 
> While jwir3's existing testcase sometimes loads correctly (e.g. with <embed>
> per comment 187) & sometimes fails, here's a variant with an initial
> "display:none" for 1 second, which delays frame-creation and makes us always
> lose the race (even in a testcase with <embed>)

Great. I'll add a mochitest for this specific problem. Hopefully, I'll have it finished today & I'll post it to the bug.

Comment 192

[Daniel Holbert [:dholbert]]

Comment on attachment 556068 [details] [diff] [review]
Patch 10 (v2) - Implementation of nsSVGFEImageElement

One nit on the nsSVGFEImageElement patch:

>+++ b/content/svg/content/src/nsSVGFilters.cpp
> private:
>+  // Flag to indicate whether or not our imgIRequest was registered with the
>+  // refresh driver.
>+  bool mRequestRegistered;
>+
>   // Invalidate users of the filter containing this element.
>   void Invalidate();
> 
>   nsresult LoadSVGImage(PRBool aForce, PRBool aNotify);
> 
> protected:
>   virtual PRBool OperatesOnSRGB(nsSVGFilterInstance*,
>                                 PRInt32, Image*) { return PR_TRUE; }

(1) generally, moz style is to put member var declarations after function declarations.

(2) I'd suggest sticking 'mRequestRegistered' in the protected block, with nsSVGFEImageElement's other member variables, to keep that class somewhat organized (member variables all together) and to make your new variable easier to find.  (The private-vs.-protected distinction doesn't really matter anyway, since it has no subclasses.   And even if there were subclasses (which there never will be), those hypothetical subclasses could override ::Init, which would mean they might want to set mRequestRegistered themselves in their own Init method.)

Comment 193

[Scott Johnson (:jwir3)]

There is a slight problem with the mRequestRegistered boolean value. It seems that some frames, such as nsImageBoxFrame, reuse the same frame with multiple image requests. So, the nsImageBoxFrame that is created to display the 'connecting' throbber is used again for the 'loading' throbber. Basically, when OnStartDecode is called again, the image request is different, but the nsImageBoxFrame is the same for both image requests - thus, the mRequestRegistered boolean value is already true. Thus, the second image doesn't get notifications for animations from the refresh driver (as it isn't registered).

I'm not sure how common this situation is, as I would think that this is a special case of reuse of the frame, as otherwise we might have different positioning information.

Comment 194

[Boris Zbarsky [:bzbarsky]]

> I'm not sure how common this situation is

In general, pretty common.  Pages set a different src on images all the time.

> as otherwise we might have different positioning information

Not sure what you mean.

Comment 195

[Scott Johnson (:jwir3)]

> In general, pretty common.  Pages set a different src on images all the time.

Sure, but does this reuse an existing frame object, or create a new one?

Comment 196

[Boris Zbarsky [:bzbarsky]]

It reuses the frame, if there's nothing else weird going on.  It does do relayout, but doesn't modify the object identity.

Comment 197

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v8) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]

Comment 198

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v3) - Implementation for nsRefreshDriver [r=roc]

Comment 199

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v4) - Implementation for nsLayoutUtils [r=roc]

Comment 200

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v11) - Implementation for nsImageFrame [r=roc]

Comment 201

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v6) - Implementation for nsImageBoxFrame [r=roc]

Comment 202

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v3) - Implementation for nsBulletFrame

Comment 203

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v2) - Implementation for nsImageLoader

Comment 204

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v2) - Implementation for nsSVGImageFrame

Comment 205

[Scott Johnson (:jwir3)]

Attachment: Patch 9 (v3) - Implementation for xul tree

Comment 206

[Scott Johnson (:jwir3)]

Attachment: Patch 10 (v3) - Implementation of nsSVGFEImageElement

Comment 207

[Scott Johnson (:jwir3)]

Attachment: Patch 11 (v10) - Implementation for RasterImage [r=dholbert,JOEDREW!]

Comment 208

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 - Test framework for all following mochitests.

Comment 209

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 - Test framework for all following mochitests.

Comment 210

[Scott Johnson (:jwir3)]

Attachment: Mochitest 1 - Basic Test for Functionality of Animated GIF Images

Comment 211

[Scott Johnson (:jwir3)]

Attachment: Mochitest 2 - Test for functionality of nsSVGImageFrame component

Comment 212

[Scott Johnson (:jwir3)]

Attachment: Mochitest 3 - Functionality test for animated images in bullets.

Comment 213

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 557533 [details] [diff] [review]
Patch 3 (v4) - Implementation for nsLayoutUtils [r=roc]

Review of attachment 557533 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +4324,5 @@
> +      // we don't need to do anything - the image request is already registered
> +      return;
> +    }
> +
> +    DeregisterImageRequest(aPresContext, *aLastRegisteredRequest, nsnull);

You can just call aPresContext->RefreshDriver()->RemoveImageRequest(aRequest) directly can't you?

@@ +4330,5 @@
> +
> +  PRBool result = aPresContext->RefreshDriver()->AddImageRequest(aRequest);
> +
> +  if (!result) {
> +    NS_WARN_IF_FALSE(result, "Unable to add image request");

NS_WARNING

Comment 214

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 557533 [details] [diff] [review]
Patch 3 (v4) - Implementation for nsLayoutUtils [r=roc]

Review of attachment 557533 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +4343,5 @@
> +/* static */
> +void
> +nsLayoutUtils::DeregisterImageRequest(nsPresContext* aPresContext,
> +                                      imgIRequest* aRequest,
> +                                      nsCOMPtr<imgIRequest>* aLastRegisteredRequest)

Oh wait ... why do we need aRequest here? Can't we just pass aLastRegisteredRequest and deregister that one?

Comment 215

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 557535 [details] [diff] [review]
Patch 4 (v11) - Implementation for nsImageFrame [r=roc]

Review of attachment 557535 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +229,5 @@
> +                              getter_AddRefs(imageRequest));
> +      nsPresContext* presContext = PresContext();
> +      NS_ASSERTION(presContext, "No PresContext");
> +      nsLayoutUtils::DeregisterImageRequest(presContext, imageRequest,
> +                                            &mLastRequestRegistered);

Like here ... seems like we don't need to get imageRequest; if a request is registered, then it'll be mLastRequestRegistered.

Comment 216

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #213)
> You can just call
> aPresContext->RefreshDriver()->RemoveImageRequest(aRequest) directly can't
> you?
Yep, I made this change.

> NS_WARNING
Changed.

Comment 217

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #214)
> Oh wait ... why do we need aRequest here? Can't we just pass
> aLastRegisteredRequest and deregister that one?
Agreed. I made this change. Thanks for pointing this out.

Comment 218

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v8) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]

Comment 219

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v3) - Implementation for nsRefreshDriver [r=roc]

Comment 220

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v4) - Implementation for nsLayoutUtils [r=roc]

Comment 221

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v11) - Implementation for nsImageFrame [r=roc]

Comment 222

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v6) - Implementation for nsImageBoxFrame [r=roc]

Comment 223

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v4) - Implementation for nsBulletFrame

Comment 224

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v3) - Implementation for nsImageLoader

Comment 225

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v3) - Implementation for nsSVGImageFrame

Comment 226

[Scott Johnson (:jwir3)]

Attachment: Patch 9 (v4) - Implementation for xul tree

Comment 227

[Scott Johnson (:jwir3)]

Attachment: Patch 10 (v4) - Implementation of nsSVGFEImageElement

Comment 228

[Scott Johnson (:jwir3)]

Attachment: Patch 11 (v11) - Implementation for RasterImage [r=dholbert,JOEDREW!]

Comment 229

[Randell Jesup [:jesup] (needinfo me)]

I made bug 129986 depend on this, since it's likely that this rewrite will either fix the 9-year-old problem or make a good testcase.  It'd be worth taking a spin of these patches on the testcase for that; it was just re-verified by someone.

Comment 230

[Scott Johnson (:jwir3)]

Attachment: Patch 10 (v5) - Implementation of nsSVGFEImageElement

Had to add an additional flag to indicate if we should try to re-register later, as in some circumstances it appears that an nsSVGDocument's presShell is not created prior to the image decoding.

Comment 231

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 557877 [details] [diff] [review]
Patch 3 (v4) - Implementation for nsLayoutUtils [r=roc]

Review of attachment 557877 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +4324,5 @@
> +      // we don't need to do anything - the image request is already registered
> +      return;
> +    }
> +
> +    aPresContext->RefreshDriver()->RemoveImageRequest(*aLastRegisteredRequest);

Isn't this going to crash when *aLastRegisteredRequest is null?

@@ +4348,5 @@
> +  NS_PRECONDITION(aPresContext, "Null PresContext detected");
> +
> +  // Deregister our imgIRequest with the refresh driver to
> +  // complete tear-down, but only if it has been registered
> +  if (!aLastRegisteredRequest) {

Shouldn't we return early here when *aLastRegisteredRequest is null?

Comment 232

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 557878 [details] [diff] [review]
Patch 4 (v11) - Implementation for nsImageFrame [r=roc]

Review of attachment 557878 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsImageFrame.cpp
@@ +626,5 @@
> +  nsPresContext* presContext = PresContext();
> +  NS_ASSERTION(presContext, "No PresContext");
> +
> +  nsLayoutUtils::RegisterImageRequest(presContext, aRequest,
> +                                      &mLastRequestRegistered);

When an <img>'s "src" attribute changes, and we start loading the new image (mPendingRequest in nsImageLoadingContent), do we get an OnStartDecode right away for the new image, while the old image is still being displayed? If so, this code will stop the old image from animating, right? Is that bad?

Comment 234

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #232)
> When an <img>'s "src" attribute changes, and we start loading the new image
> (mPendingRequest in nsImageLoadingContent), do we get an OnStartDecode right
> away for the new image, while the old image is still being displayed? If so,
> this code will stop the old image from animating, right? Is that bad?

I've verified that this is the case. OnStartDecode is called while the old image is still being displayed. Thus, the new image is still loading, but the old animation is stopped. This is bad, because it can cause hangups on sites that are loading an image, and using an animated gif as a progress bar/throbber. Unfortunately, this is kind of a catch-22, though - we need an image to be registered with the refresh driver in OnStartDecode for bug 674549, but this would force-unregister the other image. 

What we could do is implement a form of 'delayed-deregistration', so an image isn't immediately deregistered if another image becomes registered. Instead, in OnStopDecode, we could perform the deregistration. This would be a bit more cumbersome, though, because it would put the onus for deregistration of the old image on the caller, whereas right now it's taken care of in nsLayoutUtils.

Comment 235

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 - Test framework for all following mochitests.

Comment 236

[Scott Johnson (:jwir3)]

Attachment: Mochitest 1 - Basic Test for Functionality of Animated GIF Images

Comment 237

[Scott Johnson (:jwir3)]

Attachment: Mochitest 2 - Test for functionality of nsSVGImageFrame component

Comment 238

[Scott Johnson (:jwir3)]

Attachment: Mochitest 3 - Functionality test for animated images in bullets.

Comment 239

[Scott Johnson (:jwir3)]

Attachment: Mochitest 4 - Test for animations in background images

Comment 240

[Scott Johnson (:jwir3)]

Attachment: Mochitest 5 - Test for animations in SVG filters.

Comment 241

[Scott Johnson (:jwir3)]

Attachment: Mochitest 6 - Test for animations in XUL tree objects.

Comment 242

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 - Test framework for all following mochitests.

Comment 243

[Scott Johnson (:jwir3)]

Attachment: Mochitest 1 - Basic Test for Functionality of Animated GIF Images.

Comment 244

[Scott Johnson (:jwir3)]

Attachment: Mochitest 1 - Basic Test for Functionality of Animated GIF Images.

Comment 245

[Scott Johnson (:jwir3)]

Attachment: Mochitest 2 - Test for functionality of nsSVGImageFrame component.

Comment 246

[Scott Johnson (:jwir3)]

Attachment: Mochitest 3 - Functionality test for animated images in bullets.

Comment 247

[Scott Johnson (:jwir3)]

Attachment: Mochitest 4 - Test for animations in background images.

Comment 248

[Scott Johnson (:jwir3)]

Attachment: Mochitest 5 - Test for animations in SVG filters.

Comment 249

[Scott Johnson (:jwir3)]

Attachment: Mochitest 6 - Test for animations in XUL tree objects.

Comment 250

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we need to move the logic for nsImageFrame to nsImageLoadingContent and have it only operate on mCurrentRequest there. Logic that changes mCurrentRequest needs to deregister mCurrentRequest and register the new one. Similar for mImageRequest in nsImageBoxFrame. If that works, we should go back to the boolean versions of the nsLayoutUtils helpers.

Comment 251

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #250)
> I think we need to move the logic for nsImageFrame to nsImageLoadingContent
> and have it only operate on mCurrentRequest there. Logic that changes
> mCurrentRequest needs to deregister mCurrentRequest and register the new
> one. Similar for mImageRequest in nsImageBoxFrame. If that works, we should
> go back to the boolean versions of the nsLayoutUtils helpers.

Sure, I can make this change tomorrow.

Comment 252

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v5) - Implementation for nsLayoutUtils [r=roc]

Back to the original boolean version.

Comment 253

[Scott Johnson (:jwir3)]

Attachment: Patch 4 - Implementation for nsImageLoadingContent

New implementation using nsImageLoadingContent.

Comment 254

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v4) - Implementation for nsBulletFrame

Comment 255

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 558628 [details] [diff] [review]
Patch 3 (v5) - Implementation for nsLayoutUtils [r=roc]

Review of attachment 558628 [details] [diff] [review]:
-----------------------------------------------------------------

This is not the boolean version...

Comment 256

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 558629 [details] [diff] [review]
Patch 4 - Implementation for nsImageLoadingContent

Review of attachment 558629 [details] [diff] [review]:
-----------------------------------------------------------------

What you have here isn't quite right because we could start loading an image in a display:none IFRAME (where the document has no prescontext) and then later create a prescontext for it and display the image, and it wouldn't be registered/animated. (Guess we need a test for that!) It's also suboptimal because we animate display:none image elements, which isn't really necessary.

I think we should only be animating when there's a frame for the nsImageLoadingContent. Where you currently get a prescontext via the document, instead call GetPrimaryFrame() and get its prescontext if that's non-null. To handle the case where a frame is created (or destroyed), have frames for nsImageLoadingContent elements tell nsImageLoadingContent when they're created and destroyed, so you can register/unregister the requests.

Also, follow the golden rule of "don't repeat yourself" :-). There's a bunch of repeated code in this patch that could be factored out. One thing that might help is adding a helper method "bool* GetRegisteredFlagForRequest(imgIRequest* aRequest)", which would return a pointer to the right boolean depending on whether aRequest is the pending or current request.

Comment 257

[Scott Johnson (:jwir3)]

Attachment: Mochitest 7 - Test for animations where the source reference changes mid-animation.

Comment 258

[Scott Johnson (:jwir3)]

Attachment: Mochitest 8 - Test for an animated image in an (initially) undisplayed iframe.

This is to test the situation described in comment 256.

Comment 259

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v9) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]

Comment 260

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v4) - Implementation for nsRefreshDriver [r=roc]

Comment 261

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v5) - Implementation for nsLayoutUtils [r=roc]

This is _actually_ the boolean version this time. :)

Comment 262

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v2) - Implementation for nsImageLoadingContent

Comment 263

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v7) - Implementation for nsImageBoxFrame [r=roc]

Comment 264

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v5) - Implementation for nsBulletFrame

Comment 265

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v4) - Implementation for nsImageLoader

Comment 266

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v5) - Implementation for xultree

Comment 267

[Scott Johnson (:jwir3)]

Attachment: Patch 9 (v11) - Implementation for RasterImage [r=dholbert,JOEDREW!]

Comment 268

[Joe Drew (not getting mail)]

Comment on attachment 558337 [details] [diff] [review]
Mochitest 0 - Test framework for all following mochitests.

Review of attachment 558337 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/animationPolling.js
@@ +3,5 @@
> +var gIntervalID;
> +
> +function pollForSuccess ()
> +{
> +  currentTest.checkImage();

Polling for animated images is probably not what you want to do. If you want to capture a particular frame's contents, your best bet is to get the imgIRequest from the image, create an imgIDecoderObserver implementation in javascript (look in modules/libpr0n/test/mochitest/imgutils.js), then .clone() the image to get all the callbacks. If you register for onStopFrame, you'll be able to count frames/act at the right time.

The tests in modules/libpr0n/test/browser do something close to (but not exactly the same as) this.

@@ +14,5 @@
> +
> +function waitForImageLoad() {
> +  if (gIsImageLoaded) {
> +    window.clearInterval(gIntervalID);
> +  }

I don't think you need this function, do you?

@@ +65,5 @@
> + *        elements).
> + * @returns {AnimationTest}
> + */
> +function AnimationTest (pollFreq, timeout, referenceElementId, imageElementId,
> +    debugElementId, cleanId, srcAttr, xulTest)

this should be correctly indented here (line up pollFreq and debugElementId vertically. Also you don't need a space between function name and beginning of argument list.

Comment 269

[Scott Johnson (:jwir3)]

(In reply to Joe Drew (:JOEDREW!) from comment #268)
> Comment on attachment 558337 [details] [diff] [review]
> Mochitest 0 - Test framework for all following mochitests.
> 
> Review of attachment 558337 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libpr0n/test/mochitest/animationPolling.js
> @@ +3,5 @@
> > +var gIntervalID;
> > +
> > +function pollForSuccess ()
> > +{
> > +  currentTest.checkImage();
> 
> Polling for animated images is probably not what you want to do. If you want
> to capture a particular frame's contents, your best bet is to get the
> imgIRequest from the image, create an imgIDecoderObserver implementation in
> javascript (look in modules/libpr0n/test/mochitest/imgutils.js), then
> .clone() the image to get all the callbacks. If you register for
> onStopFrame, you'll be able to count frames/act at the right time.
> 
> The tests in modules/libpr0n/test/browser do something close to (but not
> exactly the same as) this.
> 
> @@ +14,5 @@
> > +
> > +function waitForImageLoad() {
> > +  if (gIsImageLoaded) {
> > +    window.clearInterval(gIntervalID);
> > +  }
> 
> I don't think you need this function, do you?
> 
> @@ +65,5 @@
> > + *        elements).
> > + * @returns {AnimationTest}
> > + */
> > +function AnimationTest (pollFreq, timeout, referenceElementId, imageElementId,
> > +    debugElementId, cleanId, srcAttr, xulTest)
> 
> this should be correctly indented here (line up pollFreq and debugElementId
> vertically. Also you don't need a space between function name and beginning
> of argument list.

Joe, Dholbert, and I discussed this on IRC. The conclusion we came to is that it is unclear whether a non-polling solution can be found. I've filed bug 685634 to follow up on this and change it if we can make it more elegant.

With respect to the formatting changes, I will make these right now.

Comment 270

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 559185 [details] [diff] [review]
Patch 4 (v2) - Implementation for nsImageLoadingContent

Review of attachment 559185 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +3239,5 @@
>      RevokeAnimationFrameNotifications();
>    }
> +
> +  // Clear image requests from refresh driver
> +  mPresShell->GetPresContext()->RefreshDriver()->ClearAllImageRequests();

Why do we need to do this here?

::: content/base/src/nsImageLoadingContent.cpp
@@ +202,5 @@
>  
> +  nsPresContext* presContext = GetOurPresContext();
> +  if (presContext) {
> +    bool* requestFlag = GetRegisteredFlagForRequest(aRequest);
> +    nsLayoutUtils::RegisterImageRequest(presContext, aRequest, requestFlag);

We don't want to register the request if there is no frame. So, I think you should call GetPrimaryFrame, and if it's non-null, call RegisterImageRequest with that frame's prescontext.

@@ +343,5 @@
> +    bool* requestFlag = GetRegisteredFlagForRequest(aRequest);
> +    nsPresContext* presContext = GetOurPresContext();
> +    if (presContext) {
> +      nsLayoutUtils::DeregisterImageRequestIfNotAnimated(presContext, aRequest,
> +                                                         requestFlag);

Same here.

@@ +535,5 @@
> +  if (mCurrentRequest) {
> +    nsLayoutUtils::RegisterImageRequest(presContext, mCurrentRequest,
> +                                        &mCurrentRequestRegistered);
> +    nsLayoutUtils::DeregisterImageRequestIfNotAnimated(presContext,
> +                                                       mCurrentRequest,

Does an image that hasn't finished decoding the first frame return false for GetAnimated even if it's animated? If so, this could unregister the request prematurely.

Maybe we need to add an API to imgIContainer --- IsNotAnimated, say --- that returns true when we know the image isn't animated, but false if we don't know yet.

Comment 271

[Joe Drew (not getting mail)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #270)
> Does an image that hasn't finished decoding the first frame return false for
> GetAnimated even if it's animated?

No - it throws NS_ERROR_NOT_AVAILABLE until one of a) the next frame has been decoded or b) the image is complete.

Comment 272

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK great.

Comment 273

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #270)

> Why do we need to do this here?

I've removed it. It was from before the nsIImageLoadingContent interface was changed to include a FrameDestroyed() method. bz instructed me that the frame should be cleaned up and deregistered prior to the presshell going away.

> We don't want to register the request if there is no frame. So, I think you
> should call GetPrimaryFrame, and if it's non-null, call RegisterImageRequest
> with that frame's prescontext.

Well, GetOurPresContext() does this, in a sort of roundabout way. It first calls GetPrimaryFrame(), and if this is null, then it returns nsnull. The check for the presContext being null should cover the situation where there isn't a frame.

I did it this way to eliminate duplicated code. I can revert back to the double null check (e.g. at each time a registration is done, call GetPrimaryFrame(), then check to see if it's null and if not, use its prescontext), if you'd prefer I do it this way.

> Same here.

I will fix both of these to the previous mentioned version if you'd prefer.

> > Does an image that hasn't finished decoding the first frame return false for
> > GetAnimated even if it's animated? If so, this could unregister the request
> > prematurely.
> > 
> > Maybe we need to add an API to imgIContainer --- IsNotAnimated, say --- that
> > returns true when we know the image isn't animated, but false if we don't
> > know yet.

> No - it throws NS_ERROR_NOT_AVAILABLE until one of a) the next frame has been decoded 
> or b) the image is complete.

So, are we ok then, or should I still make a change?

Comment 274

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v3) - Implementation for nsImageLoadingContent

Fix as per Roc's comments that removes the call to ClearAllImageRequests from nsDocument.

Comment 275

[Scott Johnson (:jwir3)]

(In reply to Joe Drew (:JOEDREW!) from comment #268)

> > +function waitForImageLoad() {
> > +  if (gIsImageLoaded) {
> > +    window.clearInterval(gIntervalID);
> > +  }
> 
> I don't think you need this function, do you?

You are correct. I've removed it from the latest patch.
 
> this should be correctly indented here (line up pollFreq and debugElementId
> vertically. Also you don't need a space between function name and beginning
> of argument list.

Fixed.

Comment 276

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 (v2) - Test framework for all following mochitests.

Comment 277

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 559531 [details] [diff] [review]
Patch 4 (v3) - Implementation for nsImageLoadingContent

Review of attachment 559531 [details] [diff] [review]:
-----------------------------------------------------------------

I feel a lot better about this patch now :-)

::: content/base/src/nsImageLoadingContent.cpp
@@ +200,5 @@
>      }
>    }
>  
> +  nsPresContext* presContext = GetOurPresContext();
> +  if (presContext) {

OK, call this GetFramePresContext(), say, to suggest that it will return a null prescontext if this content has no frame. And document that where it's declared.

@@ +522,5 @@
> +NS_IMETHODIMP
> +nsImageLoadingContent::FrameCreated(nsIFrame* aFrame)
> +{
> +  if (!aFrame) {
> +    return NS_ERROR_INVALID_ARG;

Probably just assert that aFrame is non-null, this should never happen.

@@ +528,5 @@
> +
> +  // We need to make sure that our image request is registered.
> +  nsPresContext* presContext = aFrame->PresContext();
> +  if (!presContext) {
> +    return NS_ERROR_FAILURE;

nsIFrame::PresContext() never returns null. In general, getters that don't have "Get" in the name never return null.

@@ +551,5 @@
> +NS_IMETHODIMP
> +nsImageLoadingContent::FrameDestroyed(nsIFrame* aFrame)
> +{
> +  if (!aFrame) {
> +    return NS_ERROR_INVALID_ARG;

Again, aFrame can't be null, and presContext can't be null either.

@@ +565,5 @@
> +    nsLayoutUtils::DeregisterImageRequest(presContext, mCurrentRequest,
> +                                          &mCurrentRequestRegistered);
> +  } else if (mPendingRequest) {
> +    // We don't need to do the same check for animation, because this will be
> +    // done when decoding is finished.

This comment is irrelevant here.

@@ +942,5 @@
> +nsIFrame*
> +nsImageLoadingContent::GetOurPrimaryFrame()
> +{
> +  nsCOMPtr<nsIContent> thisContent = do_QueryInterface(this);
> +  return thisContent->GetPrimaryFrame();

Can we just call this GetPrimaryFrame()? This "Our" stuff is a bit annoying :-)

@@ +1079,5 @@
> +  // notifications.
> +  nsPresContext* presContext = GetOurPresContext();
> +  if (presContext) {
> +    nsLayoutUtils::DeregisterImageRequest(presContext, mCurrentRequest,
> +                                          &mCurrentRequestRegistered);

Let's make all the nsLayoutUtils methods bail out on a null prescontext, then you can skip null checks and temporary variables here and elsewhere.

::: layout/svg/base/src/nsSVGLeafFrame.cpp
@@ +82,5 @@
> +{
> +  nsFrame::Init(aContent, aParent, asPrevInFlow);
> +  nsCOMPtr<nsIImageLoadingContent> imageLoader =
> +    do_QueryInterface(nsFrame::mContent);
> +  imageLoader->FrameCreated(this);

You must check for imageLoader being null! It definitely could be!

Comment 278

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 559181 [details] [diff] [review]
Patch 3 (v5) - Implementation for nsLayoutUtils [r=roc]

Review of attachment 559181 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +4312,5 @@
> +nsLayoutUtils::RegisterImageRequest(nsPresContext* aPresContext,
> +                                    imgIRequest* aRequest,
> +                                    bool* aRequestRegistered)
> +{
> +  NS_PRECONDITION(aPresContext, "Null PresContext detected");

Like I mentioned, I think it simplifies things to bail on null prescontext here.

@@ +4389,5 @@
> +      return;
> +    }
> +
> +    imageContainer->GetAnimated(&animated);
> +    if (!animated) {

So we need to check the nsresult of GetAnimated and if it failed, be conservative treat the image as possibly animated.

Comment 279

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v4) - Implementation for nsImageLoadingContent

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #277)
> I feel a lot better about this patch now :-)
Excellent. :)

> OK, call this GetFramePresContext(), say, to suggest that it will return a
> null prescontext if this content has no frame. And document that where it's
> declared.
Done. 

> Probably just assert that aFrame is non-null, this should never happen.
Done.

> nsIFrame::PresContext() never returns null. In general, getters that don't
> have "Get" in the name never return null.
Ok, good to know. I modified the code so that it doesn't check for this any longer.

> Again, aFrame can't be null, and presContext can't be null either.
Changed.

> This comment is irrelevant here.
Yep, I've removed it. Cut-and-paste error. :)

> Can we just call this GetPrimaryFrame()? This "Our" stuff is a bit annoying
> :-)
I completely agree, however, I originally tried to name it GetPrimaryFrame(), and it conflicts with a subclasses' implementation and causes a number of strange compiler errors. So, I took note of how it was done in other classes, and decided to call it GetOurPrimaryFrame(). If you'd like, I can try to find a more suitable method name, or see if I can force it to accept GetPrimaryFrame().

> Let's make all the nsLayoutUtils methods bail out on a null prescontext,
> then you can skip null checks and temporary variables here and elsewhere.
Done.

> You must check for imageLoader being null! It definitely could be!
I added a null check for this.

Comment 280

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v6) - Implementation for nsLayoutUtils [r=roc]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #278)
> Like I mentioned, I think it simplifies things to bail on null prescontext
> here.
Done.

> So we need to check the nsresult of GetAnimated and if it failed, be
> conservative treat the image as possibly animated.
Done.

Comment 281

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 559791 [details] [diff] [review]
Patch 4 (v4) - Implementation for nsImageLoadingContent

Review of attachment 559791 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsImageLoadingContent.h
@@ +346,5 @@
>  
> +  // Flags to indicate whether each of the current and pending requests are
> +  // registered with the refresh driver.
> +  bool mCurrentRequestRegistered;
> +  bool mPendingRequestRegistered;

This wastes two or six bytes of space because mCurrentURI and mPendingRequest have to be pointer-aligned. Move these down to the end, before or after mStateChangerDepth.

You'll have to move the constructor initialization too to avoid warnings.

Comment 282

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 559191 [details] [diff] [review]
Patch 7 (v4) - Implementation for nsImageLoader

Review of attachment 559191 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsImageLoader.cpp
@@ +73,2 @@
>  {
> +  if (mRequest && mFrame) {

Can mFrame actually be non-null here? I wouldn't have thought so.

@@ +114,5 @@
>      todestroy->Destroy();
>    }
>  
> +  if (mRequest && mFrame) {
> +    nsPresContext* presContext = mFrame->PresContext();

Likewise, can mFrame be null here?

If it can, then CancelAndForgetObserver needs to be called even if mFrame is null.

Comment 283

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 559194 [details] [diff] [review]
Patch 8 (v5) - Implementation for xultree

Review of attachment 559194 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
@@ +116,5 @@
> +
> +  // If our imgIRequest object was registered with the refresh driver,
> +  // then we need to deregister it.
> +  nsTreeBodyFrame* frame = static_cast<nsTreeBodyFrame*>(aData);
> +  if (!frame)

Can this really be null?

@@ +4701,5 @@
> +nsTreeBodyFrame::OnStartDecode(imgIRequest* aRequest)
> +{
> +  nsPresContext* presContext = PresContext();
> +  if (!presContext)
> +    return NS_ERROR_FAILURE;

Can't be null.

@@ +4714,5 @@
> +{
> +
> +  nsPresContext* presContext = PresContext();
> +  if (!presContext)
> +    return NS_ERROR_FAILURE;

Can't be null.

Comment 284

[Scott Johnson (:jwir3)]

roc, why did you mark attachment 559181 [details] [diff] [review] as obsolete?

Comment 285

[Scott Johnson (:jwir3)]

Comment on attachment 559189 [details] [diff] [review]
Patch 6 (v5) - Implementation for nsBulletFrame

Hmmm... sorry, I must have marked this one as obsolete by accident.

Comment 286

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #284)
> roc, why did you mark attachment 559181 [details] [diff] [review] as
> obsolete?

Disregard... I must have made a mistake when I posted patches last time. Too many to keep track of... ;)

Comment 287

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah, in retrospect it would have been better to finalize the hard patches first and post patches for the rest only after that was done.

Comment 288

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 559189 [details] [diff] [review]
Patch 6 (v5) - Implementation for nsBulletFrame

Review of attachment 559189 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsBulletFrame.cpp
@@ +165,5 @@
>      }
>  
>      PRBool needNewRequest = PR_TRUE;
>  
> +    mOldRequest = mImageRequest;

I don't think we need mOldRequest. If newRequest != mImageRequest you can just deregister mImageRequest and re-register newRequest.

Comment 289

[Joe Drew (not getting mail)]

Comment on attachment 559535 [details] [diff] [review]
Mochitest 0 (v2) - Test framework for all following mochitests.

Review of attachment 559535 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/animationPolling.js
@@ +3,5 @@
> +
> +function pollForSuccess ()
> +{
> +  currentTest.checkImage();
> +  setTimeout(pollForSuccess, currentTest.pollFreq);

Shouldn't we do this conditionally based on whether the test has finished?

@@ +6,5 @@
> +  currentTest.checkImage();
> +  setTimeout(pollForSuccess, currentTest.pollFreq);
> +};
> +
> +function imageLoadCallback() {

{ on new line

@@ +10,5 @@
> +function imageLoadCallback() {
> +  gIsImageLoaded = true;
> +}
> +
> +function referencePoller() {

{ on new line

@@ +78,5 @@
> +  this.onStopFrameCounter = 0;
> +  this.isTestFinished = false;
> +  this.numRefsTaken = 0;
> +  this.blankWaitTime = 0;
> +  this.cleanId = typeof(cleanId) != 'undefined' ? cleanId : '';

Is that idiomatic javascript? Can't you just say cleanid === undefined? (Presuming you want to allow empty strings as a valid cleanId; otherwise I'm almost certain you can say cleanId ? cleanId : ''; .)

@@ +86,5 @@
> +
> +  if (this.srcAttr) {
> +    this.myImage = new Image();
> +    this.myImage.src = this.srcAttr;
> +    this.myImage.onLoad = imageLoadCallback;

onload, no capital L. And it's silly, but I prefer setting it before .src, so I know I have all my ducks in a row once I start the load.

@@ +161,5 @@
> +  this.takeReferenceSnapshot();
> +
> +  // In case something goes wrong, fail earlier than mochitest timeout,
> +  // and with more information.
> +  setTimeout(failTest, this.timeout);

I'm all for failing with more information, but maybe it should be more-or-less the same timeout as the mochitest harness? I don't know how easy it is to get that timeout programmatically, though.

@@ +189,5 @@
> +  if (this.cleanId != '') {
> +    if (this.xulTest) {
> +      document.getElementById(this.imageElementId).setAttribute('hidden', 'true');
> +    } else {
> +      document.getElementById(this.imageElementId).style.display = 'none';

You do this test in several places; in some cases, it's if (this.xulTest), while in others, it's if (!this.xulTest). I think you should factor this out to a new function and then it'll be obvious what you're doing, and there'll be no reason to worry about matching up their ordering.

@@ +273,5 @@
> +      // if it took longer than two seconds to load the image, we probably
> +      // have a problem.
> +      this.wereFailures = true;
> +      ok(snapResult[0],
> +      "Reference snapshot shouldn't match clean (non-image) snapshot");

indentation or no newline here

@@ +283,5 @@
> +    }
> +  }
> +
> +  ok(snapResult[0],
> +  "Reference snapshot shouldn't match clean (non-image) snapshot");

Either indent the second argument to ok correctly here, or put it on the previous line.

@@ +313,5 @@
> +  ok(result[0], "Reference image should disappear when it becomes display:none");
> +  if (!result[0]) {
> +    this.wereFailures = true;
> +    var dataString = "Second Blank Snapshot";
> +    var debugElement = document.getElementById(this.debugElementId);

You do this big dance of setting up a bunch of new elements to output debug information in a couple of places; maybe you should write a function to do it?

Comment 290

[Joe Drew (not getting mail)]

Comment on attachment 558340 [details] [diff] [review]
Mochitest 1 - Basic Test for Functionality of Animated GIF Images.

Review of attachment 558340 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/Makefile.in
@@ +97,5 @@
>                  lime-anim-100x100.svg \
>                  test_animSVGImage.html \
> +                test_animation.html \
> +                animated-gif-frame1.gif \
> +                animated-gif-frame2.gif \

maybe animated-gif-finalframe.gif?

::: modules/libpr0n/test/mochitest/test_animation.html
@@ +34,5 @@
> +
> +function main()
> +{
> +  var animTest = new AnimationTest(20, FAILURE_TIMEOUT, 'referenceDiv',
> +      							   'animatedGif', 'debug');

indentation fix plz

Comment 291

[Joe Drew (not getting mail)]

Comment on attachment 558341 [details] [diff] [review]
Mochitest 2 - Test for functionality of nsSVGImageFrame component.

Review of attachment 558341 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/test_svg_animatedGIF.html
@@ +18,5 @@
> +<p id="display"></p>
> +<div id="content">
> +  <div id="referenceDiv" style="height: 40px; width: 40px;
> +                                display: none; background: #2aff00"></div>
> +  <embed id="embeddedSVG" src="animation.svg" type="image/svg+xml" style="display: none;"/>

add a comment to say why you chose embed over <img>

Comment 292

[Joe Drew (not getting mail)]

Comment on attachment 558907 [details] [diff] [review]
Mochitest 8 - Test for an animated image in an (initially) undisplayed iframe.

Review of attachment 558907 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/test_undisplayed_iframe.html
@@ +5,5 @@
> +-->
> +<head>
> +<title>Test for Bug 666446 - Test for Animated Gif within IFRAME</title>
> +<script type="application/javascript"
> +  src="chrome://mochikit/content/MochiKit/packed.js"></script>

This whole file looks strangely line-wrapped; I don't think it's terribly important to have only 80 character lines, so may as well just put things on the same line when it makes the most sense, like here.

@@ +23,5 @@
> +
> +  <div id="content">
> +    <div id="referenceDiv" style="display:none;">
> +      <iframe id="referenceIFrame" src="ref-iframe.html" width="50%"
> +        height="100"> Browser does not support iframes. </iframe>

Similarly, I'd prefer the whole <iframe ... > on one line, the contents indented and on a separate line, and then </iframe> unindented and on its own line too.

@@ +38,5 @@
> +
> +function main()
> +{
> +  var animTest = new AnimationTest(20, FAILURE_TIMEOUT, 'referenceDiv',
> +                       'imageIFrame', 'debug');

indentation

Comment 293

[Joe Drew (not getting mail)]

Comment on attachment 558345 [details] [diff] [review]
Mochitest 4 - Test for animations in background images.

Review of attachment 558345 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/test_background_image_anim.html
@@ +31,5 @@
> +const FAILURE_TIMEOUT = 120000; // Fail early after 120 seconds (2 minutes)
> +
> +function main() {
> +  var animTest = new AnimationTest(20, FAILURE_TIMEOUT, 'referenceDiv',
> +                       'bgImage', 'debug');

indentation

Comment 294

[Joe Drew (not getting mail)]

Comment on attachment 558346 [details] [diff] [review]
Mochitest 5 - Test for animations in SVG filters.

Review of attachment 558346 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/Makefile.in
@@ +104,5 @@
>                  test_svg_animatedGIF.html \
>                  test_bullet_animation.html \
>                  test_background_image_anim.html \
> +                filter.svg \
> +                filter2.svg \

filter-final.svg?

Comment 295

[Joe Drew (not getting mail)]

Comment on attachment 558347 [details] [diff] [review]
Mochitest 6 - Test for animations in XUL tree objects.

Review of attachment 558347 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/test_xultree_animation.xhtml
@@ +22,5 @@
> +<div id="content">
> +  	<xul:caption label="Bug 666446 - XULTree Test" />
> +	<xul:separator />
> +    <br />
> +    <xul:window id="main" title="Bug 666446: XUL Tree Testing" width="100"

this is sort of weirdly indented.

Comment 296

[Joe Drew (not getting mail)]

Comment on attachment 558906 [details] [diff] [review]
Mochitest 7 - Test for animations where the source reference changes mid-animation.

Review of attachment 558906 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/test_changeOfSource.html
@@ +47,5 @@
> +  document.getElementById('animatedGif').setAttribute('src',
> +                                                      'animated-gif2.gif');
> +  document.getElementById('animatedGif').style.display = 'none';
> +  var secondTest = new AnimationTest(20, FAILURE_TIMEOUT, 'referenceDiv',
> +		   'animatedGif', 'debug', '', '', false, true);

indentation

@@ +62,5 @@
> +
> +function main()
> +{
> +  gAnimTest = new AnimationTest(20, FAILURE_TIMEOUT, 'referenceDiv',
> +      							   'animatedGif', 'debug', '', '', false, false);

indentation

@@ +65,5 @@
> +  gAnimTest = new AnimationTest(20, FAILURE_TIMEOUT, 'referenceDiv',
> +      							   'animatedGif', 'debug', '', '', false, false);
> +  gAnimTest.beginTest();
> +
> +  gIntervalId = setInterval(checkTestFinished, 20);

So, I didn't review part 0 carefully enough given this. I think it'd be cleaner to pass a function to AnimationTest's constructor rather than having a bunch of boolean parameters; that way, if it's null or unspecified, you could ignore and finish the test, and otherwise you can call that function.

Comment 297

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v5) - Implementation for nsImageLoadingContent [r=roc]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #281)
> This wastes two or six bytes of space because mCurrentURI and
> mPendingRequest have to be pointer-aligned. Move these down to the end,
> before or after mStateChangerDepth.
> 
> You'll have to move the constructor initialization too to avoid warnings.

Done.

Comment 298

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v5) - Implementation for nsImageLoader [r=roc]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #282)
> Can mFrame actually be non-null here? I wouldn't have thought so.
Nope. I guess it's only called from within nsPresContext. I've removed this.

> Likewise, can mFrame be null here?
Same deal, I removed it.

Comment 299

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v6) - Implementation for xultree

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #283)
> Can this really be null?
Nope. I removed it.

> Can't be null.
Removed.

> Can't be null.
Removed.

Comment 300

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v7) - Implementation for xultree

Accidentally forgot a hg qref last time.

Comment 301

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 560051 [details] [diff] [review]
Patch 7 (v5) - Implementation for nsImageLoader [r=roc]

Review of attachment 560051 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsImageLoader.cpp
@@ +73,2 @@
>  {
> +  if (mRequest) {

How can mRequest be non-null here?

@@ +115,5 @@
>    }
>  
> +  if (mRequest) {
> +    nsPresContext* presContext = mFrame->PresContext();
> +    NS_ASSERTION(presContext, "Frame has no PresContext");

I don't think we really need this assertion. Frames always have a prescontext and all kinds of things would break if they didn't.

@@ +286,5 @@
> +NS_IMETHODIMP
> +nsImageLoader::OnStartDecode(imgIRequest *aRequest)
> +{
> +  if (!mFrame) {
> +    return NS_ERROR_FAILURE;

As noted before, mFrame can't be null.

@@ +308,5 @@
> +{
> +  // Deregister the imgIRequest with the refresh driver if the
> +  // image is not animated.
> +  nsPresContext* presContext = mFrame->PresContext();
> +  if (!presContext) {

Can't be null.

Comment 302

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 560079 [details] [diff] [review]
Patch 8 (v7) - Implementation for xultree

Review of attachment 560079 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
@@ +118,5 @@
> +  // then we need to deregister it.
> +  nsTreeBodyFrame* frame = static_cast<nsTreeBodyFrame*>(aData);
> +
> +  nsPresContext* presContext = frame->PresContext();
> +  NS_ASSERTION(presContext, "No PresContext");

I don't think we should bother asserting this. it just clutters up the code.

Comment 303

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v6) - Implementation for nsBulletFrame

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #288)
> I don't think we need mOldRequest. If newRequest != mImageRequest you can
> just deregister mImageRequest and re-register newRequest.
You're correct. I've removed it.

Comment 304

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #302)
> I don't think we should bother asserting this. it just clutters up the code.
I'll remove this from nsBulletFrame as well.

Comment 305

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v7) - Implementation for nsBulletFrame

Removal of useless assertions.

Comment 306

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 (v2) - Test framework for all following mochitests. [r=JOEDREW!]

(In reply to Joe Drew (:JOEDREW!) from comment #289)
> Shouldn't we do this conditionally based on whether the test has finished?
Yep, I made this change.
> { on new line
Done.

> { on new line
Done.

> Is that idiomatic javascript? Can't you just say cleanid === undefined?
> (Presuming you want to allow empty strings as a valid cleanId; otherwise I'm
> almost certain you can say cleanId ? cleanId : ''; .)
I modified it to be more clear what is happening, as you suggested.

> onload, no capital L. And it's silly, but I prefer setting it before .src,
> so I know I have all my ducks in a row once I start the load.
Changed.

> I'm all for failing with more information, but maybe it should be
> more-or-less the same timeout as the mochitest harness? I don't know how
> easy it is to get that timeout programmatically, though.
Hm... this is a comment that I took from test_animSVGImage.html - Daniel Holbert's test on animated SVG images (from which this suite of tests was originally based, although now they are quite a bit different). The test I based it on is in the same module. I could change it if we can get the mochitest timeout programmatically. I don't know whether it's totally worth it, though. If we are certain the test has failed (which we are after 120 seconds), I think we should just fail it explicitly.

> You do this test in several places; in some cases, it's if (this.xulTest),
> while in others, it's if (!this.xulTest). I think you should factor this out
> to a new function and then it'll be obvious what you're doing, and there'll
> be no reason to worry about matching up their ordering.
I extracted this out into a separate function.

> indentation or no newline here
Changed.

> Either indent the second argument to ok correctly here, or put it on the
> previous line.
Fixed.

> You do this big dance of setting up a bunch of new elements to output debug
> information in a couple of places; maybe you should write a function to do
> it?
Yep, I did. It's been extracted into outputDebugInfo() in AnimationTest.

Comment 307

[Scott Johnson (:jwir3)]

Attachment: Mochitest 1 (v2) - Basic Test for Functionality of Animated GIF Images [r=JOEDREW!]

(In reply to Joe Drew (:JOEDREW!) from comment #290)
> maybe animated-gif-finalframe.gif?
Changed.

> indentation fix plz
Done.

Comment 308

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 560192 [details] [diff] [review]
Patch 6 (v7) - Implementation for nsBulletFrame

Review of attachment 560192 [details] [diff] [review]:
-----------------------------------------------------------------

The invariants here are a bit unclear. If the invariant is "if there is a request registered, then it is mImageRequest", then nsBulletFrame::DidSetStyleContext probably needs to deregister the old mImageRequest, and nsBulletFrame::OnStartDecode/OnStopDecode should only register/unregister if aRequest == mImageRequest.

::: layout/generic/nsBulletFrame.cpp
@@ +1674,5 @@
>  
> +NS_IMETHODIMP nsBulletListener::OnStartDecode(imgIRequest *aRequest)
> +{
> +  if (!mFrame)
> +      return NS_ERROR_FAILURE;

I think NS_OK is the right result here. There's nothing wrong with not having a frame.

Comment 309

[Scott Johnson (:jwir3)]

Attachment: Mochitest 5 (v2) - Test for animations in SVG filters. [r=JOEDREW!]

(In reply to Joe Drew (:JOEDREW!) from comment #291)
> add a comment to say why you chose embed over <img>
Done.

Comment 310

[Scott Johnson (:jwir3)]

Attachment: Mochitest 8 (v2) - Test for an animated image in an (initially) undisplayed iframe. [r=JOEDREW!]

(In reply to Joe Drew (:JOEDREW!) from comment #292)
> This whole file looks strangely line-wrapped; I don't think it's terribly
> important to have only 80 character lines, so may as well just put things on
> the same line when it makes the most sense, like here.
Ok, I fixed these.

> Similarly, I'd prefer the whole <iframe ... > on one line, the contents
> indented and on a separate line, and then </iframe> unindented and on its
> own line too.
Fixed.

> indentation
Fixed.

Comment 311

[Scott Johnson (:jwir3)]

Attachment: Mochitest 4 (v2) - Test for animations in background images. [r=JOEDREW!]

(In reply to Joe Drew (:JOEDREW!) from comment #293)
> indentation
Fixed.

Comment 312

[Scott Johnson (:jwir3)]

Attachment: Mochitest 5 (v3) - Test for animations in SVG filters. [r=JOEDREW!]

(In reply to Joe Drew (:JOEDREW!) from comment #294)
> filter-final.svg?
Fixed.

Comment 313

[Scott Johnson (:jwir3)]

Attachment: Mochitest 6 (v2) - Test for animations in XUL tree objects. [r=JOEDREW!]

(In reply to Joe Drew (:JOEDREW!) from comment #295)
> this is sort of weirdly indented.
Fixed.

Comment 314

[Scott Johnson (:jwir3)]

(In reply to Joe Drew (:JOEDREW!) from comment #296)
> So, I didn't review part 0 carefully enough given this. I think it'd be
> cleaner to pass a function to AnimationTest's constructor rather than having
> a bunch of boolean parameters; that way, if it's null or unspecified, you
> could ignore and finish the test, and otherwise you can call that function.

I'm not 100% clear what you're saying here. So, (I think) what you're asking for is that instead of the boolean parameters, I should pass in a function to AnimationTest's constructor. That seems clear enough, but should this function essentially set all of these boolean parameters then? Or should the function be something similar to checkTestFinished() in this last test? I realize that this last test seems somewhat convoluted, but it's a special case of the others - we want to run more than one test.

What I could do is pass in checkTestFinished(), have AnimationTest perform the setInterval() call, then write a dummy version for the other tests that only sets up a single test, but this seems a little less intuitive to read for developers in the future. Basically, what I'm doing here is I treat each AnimationTest as a separate entity, fully encapsulated, which gets a bunch of parameters that tell it what type of test it is. Then, for the last test, I just set up two of these, and time when the second one is set up. The only tricky part is I had to add in another value to indicate whether there were more tests coming in the sequence (so it doesn't call finish() too early if there are more tests).

Comment 315

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v6) - Implementation for nsImageLoader [r=roc]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #301)
> How can mRequest be non-null here?
It can't. I removed this code.

> I don't think we really need this assertion. Frames always have a
> prescontext and all kinds of things would break if they didn't.
Removed.

> As noted before, mFrame can't be null.
Removed.

> Can't be null.
Removed.

Comment 316

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v8) - Implementation for xultree [r=roc]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #302)
> I don't think we should bother asserting this. it just clutters up the code.
Removed.

Comment 317

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v8) - Implementation for nsBulletFrame

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #308)

> The invariants here are a bit unclear. If the invariant is "if there is a
> request registered, then it is mImageRequest", then
> nsBulletFrame::DidSetStyleContext probably needs to deregister the old
> mImageRequest, and nsBulletFrame::OnStartDecode/OnStopDecode should only
> register/unregister if aRequest == mImageRequest.

That was my intended invariant. I've fixed the methods you've mentioned to work in this manner and preserve this invariant.

> I think NS_OK is the right result here. There's nothing wrong with not
> having a frame.

I changed this, but all other methods (e.g. OnStartContainer, OnDataAvailable, FrameChanged, etc...) all return NS_ERROR_FAILURE if mFrame is null. Should these be changed as well?

Comment 318

[Scott Johnson (:jwir3)]

Attachment: Mochitest 7 (v2) - Test for animations where the source reference changes mid-animation.

(In reply to Joe Drew (:JOEDREW!) from comment #296)

> indentation

Fixed.

> indentation

Fixed.

> So, I didn't review part 0 carefully enough given this. I think it'd be
> cleaner to pass a function to AnimationTest's constructor rather than having
> a bunch of boolean parameters; that way, if it's null or unspecified, you
> could ignore and finish the test, and otherwise you can call that function.

I've modified this now so that the AnimationTest constructor takes a function, which, if null, it will assume this is the last test in the sequence and finish the set of tests. If it's not null, then it will call this function instead of calling finish. (See the patch I'm about to post for Mochitest 0).

Comment 319

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 (v3) - Test framework for all following mochitests. [r=JOEDREW!]

Mochitest framework with changes requested for TEST 6.

Comment 320

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 561002 [details] [diff] [review]
Patch 6 (v8) - Implementation for nsBulletFrame

Review of attachment 561002 [details] [diff] [review]:
-----------------------------------------------------------------

> I changed this, but all other methods (e.g. OnStartContainer, OnDataAvailable, FrameChanged, etc...) all return NS_ERROR_FAILURE if mFrame is null.
> Should these be changed as well?

Yeah, you might as well change those here.

::: layout/generic/nsBulletFrame.cpp
@@ +190,5 @@
>      // No image request on the new style context
>      if (mImageRequest) {
> +      // We need to deregister the old image request
> +      nsLayoutUtils::DeregisterImageRequest(PresContext(), mImageRequest,
> +                                            &mRequestRegistered);

In the other half of the branch (when the style context has a new image request) you need to unregister the old request.

Please be more careful.

@@ +1538,5 @@
> +
> +  // First, check and see if we already have a request registered
> +  if (mImageRequest != aRequest) {
> +    // Ok, so that means that we need to re-register a new request.
> +    mRequestRegistered = false;

The invariant is "mImageRequest is registered if and only if mRequestRegistered is true", so to preserve that invariant you need to unregister it here.

@@ +1544,5 @@
> +
> +  nsPresContext* presContext = PresContext();
> +
> +  if (aRequest == mImageRequest) {
> +    nsLayoutUtils::RegisterImageRequest(presContext, mImageRequest,

Just pass PresContext() here, no need for the local

@@ +1577,5 @@
> +                                                       mImageRequest,
> +                                                       &mRequestRegistered);
> +  } else {
> +    // Deregister the old image request now that we are done decoding.
> +    nsLayoutUtils::DeregisterImageRequest(presContext, mImageRequest, nsnull);

Shouldn't you just ignore this case? Again, it seems like you're breaking your invariant.

Comment 321

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v6) - Implementation for nsImageLoadingContent [r=roc]

Caught a subtle bug that caused svg crashtests to fail. Also reworked some of the logic to maintain the invariant that 'mPendingRequestRegistered is true if and only if mPendingRequest is registered with the refresh driver and mCurrentRequestRegistered is true if and only if mCurrentRequest is registered with the refresh driver'.

Comment 322

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v7) - Implementation for nsImageBoxFrame [r=roc]

Added some logic to ensure the validity of the invariant 'mRequestRegistered is true if and only if mImageRequest is registered with the refresh driver'.

Comment 323

[Joe Drew (not getting mail)]

Comment on attachment 561016 [details] [diff] [review]
Mochitest 0 (v3) - Test framework for all following mochitests. [r=JOEDREW!]

Review of attachment 561016 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/test/mochitest/animationPolling.js
@@ +20,5 @@
> +}
> +
> +function failTest ()
> +{
> +  if (currentTest.isTestFinished || currentTest.closeFunc != undefined) {

can this just be if (currentTest.isTestFinished || currentTest.closeFunc) ?

@@ +84,5 @@
> +  this.xulTest = xulTest ? xulTest : '';
> +  if (closeFunc != undefined) {
> +    this.closeFunc = closeFunc;
> +  } else {
> +    this.closeFunc = '';

similarly here

Comment 324

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v9) - Implementation for xultree [r=roc]

Removed some unnecessary temporary variables and fixed a reftest failure that was created by modifying a hash table from within an enumeration callback.

Comment 325

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 (v4) - Test framework for all following mochitests. [r=JOEDREW!]

(In reply to Joe Drew (:JOEDREW!) from comment #323)

> can this just be if (currentTest.isTestFinished || currentTest.closeFunc) ?
Yep. I've changed it.

> similarly here
Changed this, too.

Comment 326

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v9) - Implementation for nsBulletFrame.

Comment 327

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 561623 [details] [diff] [review]
Patch 6 (v9) - Implementation for nsBulletFrame.

Review of attachment 561623 [details] [diff] [review]:
-----------------------------------------------------------------

Great.

Comment 328

[Dão Gottwald [:dao]]

Is this going to miss another release for a pending superreview?

bz, have you looked at this recently? Does the inactivity imply that you're not happy with the patch?

Comment 329

[gixxpunk]

(In reply to Dão Gottwald [:dao] from comment #328)
> Is this going to miss another release for a pending superreview?

Hopefully not. Would be great to see this fixed in FF9!

Comment 330

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This is going to miss another release because it's risky to land less than 12 hours before the merge.

Comment 331

[gixxpunk]

Hm... I don't see the point. There are six weeks in Aurora and another six weeks in Beta to iron out potential bugs.

Comment 332

[Dão Gottwald [:dao]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #330)
> This is going to miss another release because it's risky to land less than
> 12 hours before the merge.

It's riskier than landing after the merge, for sure. A premise of the release process is that things can land at any time, though. Six weeks time to be reverted on aurora if issues show up seems reasonable to me.

Anyway, the superreview on attachment 561510 [details] [diff] [review] isn't there, so this can't land.

Comment 333

[Boris Zbarsky [:bzbarsky]]

> Is this going to miss another release for a pending superreview?

Sort of.  I talked this over with Scott a few days ago, in fact; I was (and am) completely swamped with other reviews that also needed doing and more importantly wasn't quite comfortable landing this at the end of the cycle.  This is very delicate code that can lead to subtle bugs; I have no confidence in us finding them during 12 weeks of testing (nor even 18 weeks, but I'd like to have as good a shot at it as we can).

Comment 334

[Scott Johnson (:jwir3)]

(In reply to Boris Zbarsky (:bz) from comment #333)
> Sort of.  I talked this over with Scott a few days ago, in fact; I was (and
> am) completely swamped with other reviews that also needed doing and more
> importantly wasn't quite comfortable landing this at the end of the cycle. 
> This is very delicate code that can lead to subtle bugs; I have no
> confidence in us finding them during 12 weeks of testing (nor even 18 weeks,
> but I'd like to have as good a shot at it as we can).

I'm in complete agreement with bz here. This particular bug has a number of complexities that could cause problems. In fact, I'm concerned about possible problems in oranges from browser_image.js in the latest try that I pushed: https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=0f66ebd90d08 I don't think this is an intermittent orange - I think it's actually caused by (or somehow in conjunction with) these patches. 

I think we should take our time and be careful with this.

Comment 335

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v10) - Interface changes for b666446 [r=dholbert,JOEDREW!][sr=bz]

Comment 336

[Scott Johnson (:jwir3)]

Attachment: Patch 2 (v5) - Implementation for nsRefreshDriver [r=roc]

Comment 337

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v7) - Implementation for nsLayoutUtils [r=roc]

Comment 338

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v5) - Implementation for nsImageLoadingContent [r=roc]

Comment 339

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v8) - Implementation for nsImageBoxFrame [r=roc]

Comment 340

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v10) - Implementation for nsBulletFrame [r=roc]

Comment 341

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v7) - Implementation for nsImageLoader [r=roc]

Comment 342

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v10) - Implementation for xultree [r=roc]

Comment 343

[Scott Johnson (:jwir3)]

Attachment: Patch 9 (v12) - Implementation for RasterImage [r=dholbert,JOEDREW!]

Comment 344

[Scott Johnson (:jwir3)]

Attachment: Mochitest 0 (v5) - Test framework for all following mochitests. [r=JOEDREW]

Comment 345

[Scott Johnson (:jwir3)]

Attachment: Mochitest 1 (v3) - Basic Test for Functionality of Animated GIF Images [r=JOEDREW!]

Comment 346

[Scott Johnson (:jwir3)]

Attachment: Mochitest 2 (v2) - Test for functionality of nsSVGImageFrame component [r=JOEDREW!]

Comment 347

[Scott Johnson (:jwir3)]

Attachment: Mochitest 3 (v2) - Functionality test for animated images in bullets. [r=JOEDREW!]

Comment 348

[Scott Johnson (:jwir3)]

Attachment: Mochitest 4 (v3) - Test for animations in background images. [r=JOEDREW!]

Comment 349

[Scott Johnson (:jwir3)]

Attachment: Mochitest 5 (v4) - Test for animations in SVG filters. [r=JOEDREW!]

Comment 350

[Scott Johnson (:jwir3)]

Attachment: Mochitest 6 (v3) - Test for animations in XUL tree objects. [r=JOEDREW!]

Comment 351

[Scott Johnson (:jwir3)]

The up to date set of patches for this bug is available at:

https://hg.mozilla.org/users/sjohnson_mozilla.com/patches/

(Sorry for the bug spam earlier, I thought reposting to the bug was the preferred method of making my patches visible. :> )

Barring any findings during superreview of Patch 4, the posted versions are probably the final versions. For those applying the patches, you should apply in this order:

APPLY FIRST
     |
     | 1.  b666446-PATCH1
     | 2.  b666446-PATCH2
     | 3.  b666446-PATCH3
     | 4.  b666446-PATCH4
     | 5.  b666446-PATCH5
     | 6.  b666446-PATCH6
     | 7.  b666446-PATCH7
     | 8.  b666446-PATCH8
     | 9.  b666446-PATCH9
     | 10. b666446-TEST0
     | 11. b666446-TEST1
     | 12. b666446-TEST2
     | 13. b666446-TEST3
     | 14. b666446-TEST4
     | 15. b666446-TEST5
     | 16. b666446-TEST6
     | 17. b666446-TEST7
     | 18. b666446-TEST8
     |
APPLY LAST

The try push for these patches is: 
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=9b6c363481cc

The remaining oranges appear to be intermittent. Anything that I saw having to do with this try push that didn't have a bug filed on it already, and didn't go away after a rebuild, I fixed.

Comment 352

[Mats Palmgren (inactive)]

Comment on attachment 563221 [details] [diff] [review]
Patch 4 (v5) - Implementation for nsImageLoadingContent [r=roc]

> content/base/public/nsIImageLoadingContent.idl

Bump the UUID

> content/base/src/nsImageLoadingContent.cpp
> +    PRBool background = (NS_SUCCEEDED(rv) && (loadFlags & nsIRequest::LOAD_BACKGROUND));

Looks like you made it more than 80 chars wide?
If so, just leave it as is.

sr=mats

Comment 353

[Boris Zbarsky [:bzbarsky]]

The new methods on nsIImageLoadingContent need to be noscript.

What happens when FrameCreated is called multiple times on a single animated image (e.g. if it's fixed-pos and we're printing)?

Comment 354

[Scott Johnson (:jwir3)]

(In reply to Boris Zbarsky (:bz) from comment #353)
> The new methods on nsIImageLoadingContent need to be noscript.
> 
> What happens when FrameCreated is called multiple times on a single animated
> image (e.g. if it's fixed-pos and we're printing)?

bz and I discussed this in #developers. His concern was that if frameCreated() was called multiple times for the same image frame, then he wanted to make sure that it isn't registered with the refresh driver twice. 

This won't happen because we have two safeguards against it. Each image frame keeps track of the imgIRequest object that it is responsible for. It also keeps track of whether or not that imgIRequest is registered with the refresh driver for the tab in which the object is being displayed. This is mostly an efficiency tweak, but it does allow us an easy "opt out" of registration if the frame knows that the imgIRequest is already registered. 

Failing that (which is unlikely, as roc was pretty thorough in double-checking my invariants in these cases), the refresh driver actually keeps track of the imgIRequest objects that are registered with it using a hash map. So, if an imgIRequest does attempt to re-register with the refresh driver, it recognizes that this imgIRequest has already been registered, and doesn't re-register it again (the map is of nsISupportsHashKey type, so it hashes on the pointer).

bz and I agreed that this doesn't seem to be a problem, so I'm going to leave it as-is.

Comment 355

[Boris Zbarsky [:bzbarsky]]

Actually, I was concerned about frameCreated being called multiple times on the same image loading content with _different_ frames, with no intervening frameDestroyed calls, followed by a bunch of frameDestroyed at some later date.

It sounds like the refresh driver hashmap will safe us here.

Comment 356

[Scott Johnson (:jwir3)]

(In reply to Mats Palmgren [:mats] from comment #352)
> Bump the UUID
Done.

> Looks like you made it more than 80 chars wide?
> If so, just leave it as is.
Yes, I did. I believe I did this because it wasn't clear where I should break the line there.

> sr=mats
Thank you.

Comment 357

[Daniel Holbert [:dholbert]]

Landed at Scott's request: (parts 1 through 18)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1da3cfb954b1
https://hg.mozilla.org/integration/mozilla-inbound/rev/a48ccb52c137
https://hg.mozilla.org/integration/mozilla-inbound/rev/a476b7988f63
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d3ac2d3da11
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ec20f190f00
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd4f2d8c2f9
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4dd36b1f78
https://hg.mozilla.org/integration/mozilla-inbound/rev/49d997bcdc60
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a32a21af687
https://hg.mozilla.org/integration/mozilla-inbound/rev/94252c34312e
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6488a41e72e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fc2cff5bc6e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbf08c5aa833
https://hg.mozilla.org/integration/mozilla-inbound/rev/11c3d02d1c0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/9669c49bdf67
https://hg.mozilla.org/integration/mozilla-inbound/rev/e02aa8977225
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a62ca6580da
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae1ba25d7d07

Comment 358

[Boris Zbarsky [:bzbarsky]]

Uh... what happened to making the nsIImageLoadingContent methods noscript?

Comment 359

[Boris Zbarsky [:bzbarsky]]

And actually, as long as we're there, notxpom too....

Comment 360

[Scott Johnson (:jwir3)]

(In reply to Boris Zbarsky (:bz) from comment #358)
> Uh... what happened to making the nsIImageLoadingContent methods noscript?

Whoops, sorry bz. I thought comment 355 was two things addressing the same issue... I didn't realize they were two separate problems. 

I'm compiling a followup patch for this right now.

Comment 361

[Scott Johnson (:jwir3)]

Attachment: UPDATE 1 - Make the nsIImageLoadingContent methods frameCreated and frameDestroyed inaccessible from script.

Comment 362

[Daniel Holbert [:dholbert]]

Comment on attachment 564384 [details] [diff] [review]
UPDATE 1 - Make the nsIImageLoadingContent methods frameCreated and frameDestroyed inaccessible from script.

Looks good, aside from the s/void/NS_IMETHODIMP_(void)/ tweak to the .cpp files, as mentioned in IRC.

Landed with that fixed:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/32676bb1968e

Comment 363

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/1da3cfb954b1
through
https://hg.mozilla.org/mozilla-central/rev/ae1ba25d7d07

Comment 364

[Daniel Holbert [:dholbert]]

Reopening until the followup makes it over, too.  (that should probably happen before tonight's nightly is cut)

Comment 365

[Daniel Holbert [:dholbert]]

Merged followup to m-c:
 https://hg.mozilla.org/mozilla-central/rev/32676bb1968e

Comment 366

[Joe Drew (not getting mail)]

\o/

Comment 367

[Boris Zbarsky [:bzbarsky]]

This caused significant performance regression on tscroll (25%) and tdhtml (32%) on WinXP.  Interestingly, not elsewhere...

Comment 368

[Scott Johnson (:jwir3)]

(In reply to Boris Zbarsky (:bz) from comment #367)
> This caused significant performance regression on tscroll (25%) and tdhtml
> (32%) on WinXP.  Interestingly, not elsewhere...

Have bugs been filed for these, or should I file them?

Comment 369

[Boris Zbarsky [:bzbarsky]]

Nothing filed yet; go for it.

Comment 370

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 563229 [details] [diff] [review]
Patch 9 (v12) - Implementation for RasterImage [r=dholbert,JOEDREW!]

>--- a/modules/libpr0n/src/RasterImage.h
>+++ b/modules/libpr0n/src/RasterImage.h
>+ * It would be nice if we had a macro for this in prtypes.h.
>+ * TODO: Place this macro in prtypes.h as PR_UINT64_MAX.
>+ */
>+#define UINT64_MAX_VAL PRUint64(-1)

There are a number of those #defines already, can you file and fix this?

Comment 371

[Scott Johnson (:jwir3)]

(In reply to Ms2ger from comment #370)
> There are a number of those #defines already, can you file and fix this?

Yes. This has already been opened as bug 674277. I've got a local patch that I'll post to that bug later this week.

Comment 372

[Scott Johnson (:jwir3)]

(In reply to Boris Zbarsky (:bz) from comment #369)
> Nothing filed yet; go for it.

Added as bug 691775.

Comment 373

[dindog]

significant improvement... great works!

Comment 374

[Alon Zakai (:azakai)]

This is excellent! Thanks Scott and everyone else involved!

Comment 375

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Boris Zbarsky (:bz) from comment #367)
> This caused significant performance regression on tscroll (25%) and tdhtml
> (32%) on WinXP.  Interestingly, not elsewhere...

Maybe we should back out for this while we track it down?

A regression this large shouldn't be that hard to track down.

Comment 376

[[e-mail]]

Something in one of these patches seems to have broken rendering of resized GIFs. Example: http://www.furaffinity.net/journal/2263796/ (the mouth in the grid of hyena icons renders incorrectly in the Windows Nightly 2011-10-06 build)

Comment 377

[Scott Johnson (:jwir3)]

(In reply to [e-mail] from comment #376)
> Something in one of these patches seems to have broken rendering of resized
> GIFs. Example: http://www.furaffinity.net/journal/2263796/ (the mouth in the
> grid of hyena icons renders incorrectly in the Windows Nightly 2011-10-06
> build)

I've added a new bug to track this regression: bug 692573. It looks as though it's only rendering incorrectly in the page, though - if you view the image directly, it's rendering just fine.

Comment 378

[[e-mail]]

(In reply to Scott Johnson (:jwir3) from comment #377)
> (In reply to [e-mail] from comment #376)
> > Something in one of these patches seems to have broken rendering of resized
> > GIFs. Example: http://www.furaffinity.net/journal/2263796/ (the mouth in the
> > grid of hyena icons renders incorrectly in the Windows Nightly 2011-10-06
> > build)
> 
> I've added a new bug to track this regression: bug 692573. It looks as
> though it's only rendering incorrectly in the page, though - if you view the
> image directly, it's rendering just fine.

That's why I mentioned the "resized" bit - the icons on that page are resized, that's what the site does when you link an avatar in a comment. The issue isn't shown in full-sized gifs, including the direct-view version of resized ones that are affected. I don't really know where else to look for more examples of resized, animated GIFs other than digging around in journals some more.

Comment 379

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we should back this out while we work on the regressions.

Comment 380

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #379)
> I think we should back this out while we work on the regressions.

I agree, but I don't currently have the power to back things out of m-c or m-i right now, though, so if someone could do that, it would be great. :)

Comment 381

[Daniel Holbert [:dholbert]]

Backed out the 18 csets from comment 357 and the one followup from comment 362, in reverse, with this command for each one:
> hg backout $x; hg commit -u "Scott Johnson <[e-mail]>" -m "Backout cset $x from bug 666446 while we sort out regressions"

No merge conflicts (woot). Backout csets are as follows:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abc08c0daf9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/616b33122a7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c91fccbff79
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c78f35fa440
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8afe7189002
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfc8586375a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/c7192470011b
https://hg.mozilla.org/integration/mozilla-inbound/rev/370048394dd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/be54831e6e0d
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a61c1c8ddf4
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f1bd9bef43f
https://hg.mozilla.org/integration/mozilla-inbound/rev/8191fa520f85
https://hg.mozilla.org/integration/mozilla-inbound/rev/f590664039fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4813a3c0ce0
https://hg.mozilla.org/integration/mozilla-inbound/rev/136951acb906
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c4958bb4256
https://hg.mozilla.org/integration/mozilla-inbound/rev/c94c75d79b0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/a80d383fb026
https://hg.mozilla.org/integration/mozilla-inbound/rev/d051a5749802

Comment 382

[Matt Brubeck (:mbrubeck)]

Backout merged to mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/d051a5749802 and ancestors

Comment 384

[Scott Johnson (:jwir3)]

Just wanted to keep everyone apprised of what's happening with this bug. I've fixed the other regressions, aside from bug 691775. One of these, bug 691792, has been integrated into the patch I created for nsImageListener. The other one, bug 692573, is pending review from joe, then it will be integrated into the RasterImage patch. Since they are such small changes, I thought it better to have them reviewed as separate patches, and then integrated, rather that requesting review for the entire patch again (hopefully this will lead to a speedier review).

As for the talos regressions in bug 691775, I am working on profiling it on windows xp right now. I have a VM with XP set up on my local machine, along with the test data from the talos suite. My hope is that I can use a profiler like verysleepy to get an idea of where time is being spent for the talos scroll and dhtml tests. Unfortunately, it looks like I can't use xperf on anything older than Windows Vista. I saw that it is possible to copy the executable over and run it, and then display the data on a newer machine, but everything I've read says it won't give stack trace data, so it seems somewhat like this isn't the tool I can use. 

I also tried using AMD's CodeAnalyst, but because my machine has an intel Core i7 in it, it seems to be giving me unexpected blue screens/restarts within 2 minutes of the VM starting up. 

If anyone has experience in profiling on Windows XP, I'd be really excited to hear any advice you might have!

Comment 385

[Robert O'Callahan (:roc) (email my personal email if necessary)]

First thing is to look at the Talos results and see if there are any particular pages that regressed particularly badly. Then see if you can reproduce that regression in your VM. If you can, we should be in decent shape. You could dig into the page and maybe even turn it into a simplified testcase.

Comment 386

[Scott Johnson (:jwir3)]

Attachment: Standalone Talos results

Ok, so I've run the standalone talos tests on a windows XP virtual machine, with and without the patches applied. I ran each set of tests 7 times to be sure that I had accurate results, and came up with the attached spreadsheet. 

It looks to me as though there isn't a significant difference with the tscroll tests. It seems to me that this could be lack of reproducibility on the part of the actual regression... either than, or perhaps there was some other fluke that happened on m-c's talos runs... I'm not really sure how to proceed with this one.

For the tdhtml tests, there is a difference, but the difference of averages appears to be only about 4%, but there appears to be a ~28% difference in the slidein.html test, which I'm going to look into to see if I can determine what's going on with that test.

None of these, aside from that one slidein.html test, appear to be anywhere near the numbers we were seeing on the m-c talos tests. Is it possible that one of the other patches that was pushed up with these patches might have caused this? I'm just not really sure why this wouldn't be consistent across all test environments.

Comment 387

[Justin Lebar (not reading bugmail)]

> I'm just not really sure why this wouldn't be consistent across all test environments.

You can ask releng for direct access to a Talos machine if you want to rule this out.

Comment 388

[Scott Johnson (:jwir3)]

(In reply to Justin Lebar [:jlebar] from comment #387)
> You can ask releng for direct access to a Talos machine if you want to rule
> this out.

Thanks for letting me know. I didn't know this - I've made this request.

Comment 389

[Robert O'Callahan (:roc) (email my personal email if necessary)]

One thing you should check is whether in Tdhtml the number of paints has changed a lot. It's possible that you've enabled us to paint more smoothly during Tdhtml at the cost of some time to run the test.

You might be able to test this using tryserver pushes.

Comment 390

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #389)
> One thing you should check is whether in Tdhtml the number of paints has
> changed a lot. It's possible that you've enabled us to paint more smoothly
> during Tdhtml at the cost of some time to run the test.
> 
> You might be able to test this using tryserver pushes.

In order to instrument this a bit, I compiled the code both with and without the patches and added a print() to nsWindowGfx::OnPaint() on a windows XP machine. The results I had after successively loading all of the pages of tscroll and tdhtml were (I averaged these over five iterations of tests):

WITH PATCHES:
=================
dhtml: 470 paints
scroll: 456 paints

WITHOUT PATCHES:
================= 
dhtml: 448 paints
scroll: 422 paints

So it looks like there is about a 5% increase in the number of paints when the patches are added with the dhtml tests, and an 8% increase in the number of paints when the patches are applied with the scroll tests. 

I will say that since I ran these tests by hand (in order to determine which of them might be causing a slowdown), I noticed a couple of things:

1) With both sets of test cases, if I immediately re-ran firefox after closing it from a previous set of tests, successive tests were significantly slower than if I waited for a minute or two before reopening firefox.

2) The dhtml tests without the patches applied were jerky, and seemed to be painting fewer frames overall. The same tests with the patches applied were smoother, and the animations more pronounced (rather than simply appearing as a 'jump' from one part of the canvas to another).

3) With the scroll tests, the 'reader' test and the 'tiled-downscale' tests took much longer to complete than any of the other tests. 

I also ran a set of talos tests on this machine, as I did on the virtual machine, and will be posting that shortly.

Comment 391

[Scott Johnson (:jwir3)]

Attachment: Patch 10 - Combined mochitests to test functionality for this bug. [r=joe]

I combined all of the previous patches for tests 0-6 into a single patch.

Comment 392

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Scott Johnson (:jwir3) from comment #390)
> 1) With both sets of test cases, if I immediately re-ran firefox after closing it > from a previous set of tests, successive tests were significantly slower than if > I waited for a minute or two before reopening firefox.

This is weird and unexpected.

> 2) The dhtml tests without the patches applied were jerky, and seemed to be
> painting fewer frames overall. The same tests with the patches applied were
> smoother, and the animations more pronounced (rather than simply appearing
> as a 'jump' from one part of the canvas to another).

Can you quantify this by logging timestamps and dirty regions for each paint, disregard paints that don't update the content window, and then plot the timestamps for the paints that update the content window?

Comment 393

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #392)
> This is weird and unexpected.

Agreed. Maybe it's just my perception.

> Can you quantify this by logging timestamps and dirty regions for each
> paint, disregard paints that don't update the content window, and then plot
> the timestamps for the paints that update the content window?

Sure. I'll get right on this and post the results as soon as I have them.

Comment 394

[Scott Johnson (:jwir3)]

Attachment: Paints Plotted

I plotted the number of paints over time, both with and without the patches applied. This is a histogram of the number of paints that occur, given a 1000 ms time period (i.e. how many paints happened during intervals of 1s).

So, there are definitely periods where there are more paints happening when the patches are applied, but it doesn't seem to be abnormally different. The biggest differences seem to be at about 12s and 74s. Also, the test runs without the patches applied between about 34s and 37s appear to be much higher than the number of paints performed at the same time with the patches applied. That said, because the set of tests with the patches applied took longer to run than the other set of tests, they aren't going to match up exactly along the x-axis.

The total number of paints with the patches applied is about 566 and the total number without the patches applied is about 555. 

I'm still in the process of analyzing this data, and determining what's going on at the points of interest.

Comment 395

[Scott Johnson (:jwir3)]

I thought I would give another quick update. After several stages of try pushes, I've narrowed the performance regression to the patch with nsImageLoader, controlling image animation for background images and border images. Specifically, without the registration/deregistration calls in nsImageLoader, the problem goes away. 

My hypothesis is that perhaps images aren't getting correctly deregistered when they aren't animated. None of the images in the tscroll or tdhtml tests are animated images, so they should be deregistered when decoding is finished. 

However, this doesn't explain why it would be happening on Windows XP only. If this were the case, then it seems as though these tests would be running slower on all platforms. My thinking on this is that perhaps Windows XP paints a bit slower than other platforms - that is, the other platforms can handle the load of the refresh driver's ticks and animation updates better than Windows XP. I'm still investigating this, but my hope is to have an answer soon.

Comment 396

[Dão Gottwald [:dao]]

> However, this doesn't explain why it would be happening on Windows XP only.
> If this were the case, then it seems as though these tests would be running
> slower on all platforms. My thinking on this is that perhaps Windows XP
> paints a bit slower than other platforms - that is, the other platforms can
> handle the load of the refresh driver's ticks and animation updates better
> than Windows XP. I'm still investigating this, but my hope is to have an
> answer soon.

Bug 660264 perhaps?

Comment 397

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Scott Johnson (:jwir3) from comment #395)
> My hypothesis is that perhaps images aren't getting correctly deregistered
> when they aren't animated. None of the images in the tscroll or tdhtml tests
> are animated images, so they should be deregistered when decoding is
> finished.

That should be easy enough to test, right? Even if you don't trust the results of running locally, you could add some logging code to nsImageLoader and then examine output in the logs from tryserver.

> However, this doesn't explain why it would be happening on Windows XP only.
> If this were the case, then it seems as though these tests would be running
> slower on all platforms. My thinking on this is that perhaps Windows XP
> paints a bit slower than other platforms - that is, the other platforms can
> handle the load of the refresh driver's ticks and animation updates better
> than Windows XP. I'm still investigating this, but my hope is to have an
> answer soon.

I wouldn't worry too much about this. There are lots of possibly reasons why regressions don't matter or don't show up on some set of platforms.

Comment 398

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #397)
> That should be easy enough to test, right? Even if you don't trust the
> results of running locally, you could add some logging code to nsImageLoader
> and then examine output in the logs from tryserver.

Yes. I did this exact set of tests, and found what I think to be the problem. Under some conditions, the images loaded for background images or border images in nsImageLoader weren't being properly deregistered. I pushed a fix to try-server, and it came back that the performance regressions have subsided when this is taken into consideration. I'm polishing this up and then I will be posting a patch for review soon. This should be the last of the regressions for this bug, and hopefully it can land early this week.

Comment 399

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v8) - Implementation for nsImageLoader, with fixes for performance regression

Ok, I think the performance regression has been fixed with this new version of the nsImageLoader patch that verifies that mRequest is deregistered properly on all situations where it should be. This seemed to be causing more requestRefresh() events to occur than should have been occurring. 

The fix can be seen in this try server push:
https://tbpl.mozilla.org/?tree=Try&rev=329882dfc975

Comment 400

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 570586 [details] [diff] [review]
Patch 7 (v8) - Implementation for nsImageLoader, with fixes for performance regression

Review of attachment 570586 [details] [diff] [review]:
-----------------------------------------------------------------

So basically you've added code to Load() and to the destructor. The changes to Load() look good. Why do the changes to the destructor matter --- can we actually reach the destructor without calling Destroy() first? That sounds bad.

::: layout/base/nsImageLoader.cpp
@@ +140,5 @@
> +  // Deregister mRequest from the refresh driver, since it is no longer
> +  // going to be managed by this nsImageLoader.
> +  nsPresContext* presContext = mFrame->PresContext();
> +
> +  if (presContext) {

presContext can't be null.

@@ +154,5 @@
>    mRequest.swap(newRequest);
> +
> +  // Re-register mRequest with the refresh driver, but immediately deregister
> +  // if it isn't animated.
> +  if (presContext) {

presContext can't be null.

Comment 401

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #400)

> So basically you've added code to Load() and to the destructor. The changes
> to Load() look good. Why do the changes to the destructor matter --- can we
> actually reach the destructor without calling Destroy() first? That sounds
> bad.

Yes, I think we can. The following code:
    
nsCOMPtr<nsImageLoader> img;
img = nsImageLoader::Create(this, mImageRequest, 0, nsnull);
img = nsnull;

will call the destructor (from within Release()), but not Destroy(). I set a conditional breakpoint in Destroy(), after img is originally created, so that it will break when this == <address of img.mRawPtr>. Unless I place an explicit img->Destroy() call in before img = nsnull, the breakpoint never gets hit. 

The original reason I noticed this was because I saw that the following:

  if (mRequest) {
    mRequest->CancelAndForgetObserver(NS_ERROR_FAILURE);
  }

was in both the destructor and Destroy(). If Destroy() always preceded the destructor, then this would be redundant, and the destructor should be empty, as in nsImageBoxFrame.

Now, that said, in our code, we probably always have a preceding call to Destroy() prior to actually setting the pointer to nsnull, forcing the destructor to be called, but this is something the developer would have to remember to do in advance. Perhaps we should restructure this so that Destroy() is always called from the destructor, as part of tear-down?

> presContext can't be null.
I've fixed these occurrences, and will post a new patch as soon as we have a chance to discuss the above issue.

Comment 402

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #401)

> Yes, I think we can. The following code:

However, regardless of this, as can be seen in the latest try push that I did:
https://tbpl.mozilla.org/?tree=Try&rev=118f835410d2

removing the code from the destructor doesn't affect performance. So, it appears that everywhere where nsImageLoader is destroyed, Destroy() is called before the destructor.

Comment 403

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Scott Johnson (:jwir3) from comment #401)
> Yes, I think we can. The following code:
>     
> nsCOMPtr<nsImageLoader> img;
> img = nsImageLoader::Create(this, mImageRequest, 0, nsnull);
> img = nsnull;
> 
> will call the destructor (from within Release()), but not Destroy(). I set a
> conditional breakpoint in Destroy(), after img is originally created, so
> that it will break when this == <address of img.mRawPtr>. Unless I place an
> explicit img->Destroy() call in before img = nsnull, the breakpoint never
> gets hit.

Sure, but the question is whether in the current code it's possible for the destructor to run before Destroy() is called.

> The original reason I noticed this was because I saw that the following:
> 
>   if (mRequest) {
>     mRequest->CancelAndForgetObserver(NS_ERROR_FAILURE);
>   }
> 
> was in both the destructor and Destroy(). If Destroy() always preceded the
> destructor, then this would be redundant, and the destructor should be
> empty, as in nsImageBoxFrame.

"It's just redundant" is a definite possibility :-).

> Now, that said, in our code, we probably always have a preceding call to
> Destroy() prior to actually setting the pointer to nsnull, forcing the
> destructor to be called, but this is something the developer would have to
> remember to do in advance. Perhaps we should restructure this so that
> Destroy() is always called from the destructor, as part of tear-down?

Yes, I think we should just do that.

(In reply to Scott Johnson (:jwir3) from comment #402)
> However, regardless of this, as can be seen in the latest try push that I
> did:
> https://tbpl.mozilla.org/?tree=Try&rev=118f835410d2
> 
> removing the code from the destructor doesn't affect performance. So, it
> appears that everywhere where nsImageLoader is destroyed, Destroy() is
> called before the destructor.

There might be a small number of objects that are destroyed without Destroy() being called, in which case performance wouldn't be noticeably affected. So this doesn't prove anything.

Comment 404

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v9) - Implementation for nsImageLoader, with fixes for performance regression

Fixed issues described in comment 403.

Comment 405

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 570879 [details] [diff] [review]
Patch 7 (v9) - Implementation for nsImageLoader, with fixes for performance regression

Review of attachment 570879 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsImageLoader.cpp
@@ +102,5 @@
>      todestroy->mNextLoader = nsnull;
>      todestroy->Destroy();
>    }
>  
> +  if (mRequest) {

if (mRequest && mFrame) for safety

Comment 406

[Scott Johnson (:jwir3)]

Submitted to try with positive results:

https://tbpl.mozilla.org/?tree=Try&rev=f08f492ead84

All oranges have been verified to be either intermittent, related to a previous try-push, or were re-run to make them green.

Comment 407

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v11) - Interface changes for b666446 [r=dholbert,joe][sr=bz]

Comment 408

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v6) - Implementation for nsImageLoadingContent. [r=roc][sr=mats]

Comment 409

[Justin Lebar (not reading bugmail)]

Checked in patches from rev 3d90ebfd79d7 of http://hg.mozilla.org/users/sjohnson_mozilla.com/patches/

https://hg.mozilla.org/integration/mozilla-inbound/rev/84d20e63f8bd
https://hg.mozilla.org/integration/mozilla-inbound/rev/acb7f26bd0e4
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d75d91bf41
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a6c014fb5a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aee4462ab3c
https://hg.mozilla.org/integration/mozilla-inbound/rev/83fd4bdf6469
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb8c432c0092
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cfeffcdec35
https://hg.mozilla.org/integration/mozilla-inbound/rev/5596034f204b
https://hg.mozilla.org/integration/mozilla-inbound/rev/71aa042699d2

Comment 410

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/84d20e63f8bd
https://hg.mozilla.org/mozilla-central/rev/acb7f26bd0e4
https://hg.mozilla.org/mozilla-central/rev/79d75d91bf41
https://hg.mozilla.org/mozilla-central/rev/7a6c014fb5a6
https://hg.mozilla.org/mozilla-central/rev/2aee4462ab3c
https://hg.mozilla.org/mozilla-central/rev/83fd4bdf6469
https://hg.mozilla.org/mozilla-central/rev/cb8c432c0092
https://hg.mozilla.org/mozilla-central/rev/8cfeffcdec35
https://hg.mozilla.org/mozilla-central/rev/5596034f204b
https://hg.mozilla.org/mozilla-central/rev/71aa042699d2

(Though for anyone else following along at home, bug 691775 has had to be reopened as a result, presume any decision on whether this sticks will be made there)

Comment 411

[Mark Straver]

"Target Milestone: --- ➔ mozilla10"

Is there any chance we could please see this implemented in v8 or v9 so we don't have to wait 6 months for this very annoying issue to be fixed? pretty please? I'd rather have a 3.3% regression in tscroll than a saturated core because of animated images ;)

Comment 412

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

No, this is pretty invasive, and there's a large regression potential.

You also won't have to wait 6 months.  Thanks to the magic of rapid release, this should be in stable builds in about 13 weeks (unless it gets backed out again).

Comment 413

[Justin Lebar (not reading bugmail)]

Firefox 10 is scheduled to be released on 1/31/2011.

If it's a problem for you, you can always run Aurora, which will have this fix next week!

https://wiki.mozilla.org/RapidRelease/Calendar

Comment 414

[Justin Lebar (not reading bugmail)]

Backed out, per Scott's request.

https://hg.mozilla.org/mozilla-central/rev/fe4461c76541
https://hg.mozilla.org/mozilla-central/rev/b8dd6f6f4207

Comment 415

[IU]

(In reply to Justin Lebar [:jlebar] from comment #414)
> Backed out, per Scott's request.

What's the reason?

Comment 416

[Daniel Holbert [:dholbert]]

Possible performance regression on Windows XP.  See bug 691775 comment 2 and after.

Comment 417

[Scott Johnson (:jwir3)]

(In reply to IU from comment #415)
> What's the reason?

A bit more information -

We noticed a performance regression on Windows XP, as mentioned by Ed in comment 410. As part of the investigation of this, bug 691775. After discussing it on IRC for a bit, we came to the conclusion that the best course of action is to take a slightly different approach in implementing the resolution, and not cover quite as much ground with this rearchitecture as we had originally planned. 

Our original intention was to make images subscribe to notifications for refresh with the refresh driver under the following conditions: 1) the image was decoded and is animating (has more than 1 frame), or 2) the image is decoding. As Kyle mentioned, this bug has enormous potential for regressions, and could possibly set us back quite a bit if we're not careful. As such, we've decided to be a bit more conservative, in case there's something else going on none of us are seeing, and implement this in a way that solves the problem of 1) without extending our reach for 2). 

This decision was reached because we believe it safeguards us against possible really bad regressions, as yet unknown or invisible, in the future. I'll be working on this as quickly as I can, so it will be in the release as soon as it can be correctly and completely implemented.

Comment 418

[gixxpunk]

This 3,3% regression is a joke compared to Firefox beeing unusable on my old laptop when browsing some forum because of cpu lockdown.

Comment 419

[Dão Gottwald [:dao]]

I don't know this code, but my feeling is that you're too afraid of shipping something imperfect. What we ship right now is *far* from perfect, as the previous comment indicates. I think this could have stayed longer on trunk, even be merged to aurora, and it could have been backed out if a major regression showed up. In parallel you could have worked on refactoring this code for the next release and future sanity.

Comment 420

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We have a no-regressions policy and generally I think it serves us well. If we had no idea how to address the regressions we'd have to think hard about the tradeoffs here, but we do have a plan to make the patch even better and it shouldn't take long to do another iteration, so I don't think we need to have known-regressy code on trunk in the meantime.

Comment 421

[Phoenix]

Yup. Users will wait another year for fix, that is only their problem with full core usage and hangs, right...

Comment 422

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Six weeks max.

Comment 423

[Phoenix]

I'll remember that ;)

Comment 424

[Mark Straver]

I thought the idea was to have (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #420)
> We have a no-regressions policy and generally I think it serves us well. 

Apart from when it comes to the regression we're having this bug for, in the first place?

> If
> we had no idea how to address the regressions we'd have to think hard about
> the tradeoffs here, but we do have a plan to make the patch even better and
> it shouldn't take long to do another iteration, so I don't think we need to
> have known-regressy code on trunk in the meantime.

So, basically, you'd rather keep a very involved patch that fixes most of the issues (including unusable browsers on single-core machines) off the trunk/aurora because the patch isn't perfectly polished, looking at "planned" fixes in 6 weeks - and hold off because of:

> possible really bad regressions, as yet unknown or invisible

...that aren't as of yet a problem, and can be fixed with an updated patch later on?

I thought the whole idea of having Aurora is to be able to push non-polished bugfixes that have passed review and see -if- regressions or major issues happen in the "wild", but maybe I was wrong :)

Comment 425

[dindog]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #420)
> We have a no-regressions policy and generally I think it serves us well.
My humble idea:
This bug595671 is one of the biggest regressions Firefox4 had. Especially after many memory issues is fixed. The potential patch regression is a minor one comparing what it fix. Six weeks probaly will let this issue haunts Firefox 9...

If I have the chance to make decision, then I would landed it, and monitor is there servere regression besides the 3% xp issue. Meanwhile, work on the new patch to have it fixed once and ever in future.

Comment 426

[q0131983]

Hello, I'm using plurk with emotion script here: http://citytalk.tw/bbs/forum.php?mod=viewthread&tid=22156 (sorry for Chinese, only need Greasemonkey first than install http://citytalk.tw/ff.user.js , and open plurk it will show)
It has many gif emotions, and some of them are made of lots of frames. It always takes a lot of CPU usage (and slow) on Firefox but good on other browsers.

But I found that only version around 2011-10-06 make these lots frames gifs looks normal with decent resources use. Other versions after or before that date can't get the same result as I tried. No other changes on settings when testing between different versions.

Don't know why, but I think maybe the Attachment #564384 [details] [diff] has something to do with this.

Comment 427

[q0131983]

(update)
The nightly build of
2011-11-03-04-05-01-ux and 2011-11-03-04-04-51-jaegermonkey
both are ok now!! Running heavy gifs as smooth as 2011-10-06 versions.

But 2011-11-03-04-03-33-fx-team and 2011-11-03-03-11-31-mozilla-central
are still slow though.

Comment 428

[Justin Lebar (not reading bugmail)]

(In reply to q0131983 from comment #427)
> (update)
> The nightly build of
> 2011-11-03-04-05-01-ux and 2011-11-03-04-04-51-jaegermonkey
> both are ok now!! Running heavy gifs as smooth as 2011-10-06 versions.
> 
> But 2011-11-03-04-03-33-fx-team and 2011-11-03-03-11-31-mozilla-central
> are still slow though.

That's because we landed this bug in mozilla-central and then backed it out due to the performance regression.

Comment 429

[CruNcher]

Why not leave it at least in Nightly and see what happens ;)

Comment 430

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v8) - Implementation for nsLayoutUtils.

Roc, 

I've added a new method, similar to DeregisterIfNotAnimated, to nsLayoutUtils, to encapsulate some of the conditional registration we were talking about yesterday (specifically, in OnStopFrame). However, there isn't an obvious way for me to get the number of decoded frames from an imgIContainer (I could call GetFrame() twice, and see if it returns an error, but that seemed less than elegant, and it doesn't guarantee that there won't be another frame decoded later). 

So, what I did was force it to register if 1) it's finished decoding and GetAnimated returns that it is animated, or 2) it's not finished decoding, in which case, if it's not animated, it will be handled in OnStopDecode in the layout classes.

Let me know what you think about this.

Comment 431

[Robert O'Callahan (:roc) (email my personal email if necessary)]

// Right now, this check uses GetAnimated(), which is only guaranteed
// if the image is fully decoded.

I'm not sure what this means. GetAnimated should fail until we know whether the image is animated, then it should succeed and return true if and only if the image is animated. So we should just be able to use it, we shouldn't have to do anything clever.

If GetAnimated isn't returning success early enough in the image's life cycle, then we should fix that in imagelib.

Also I think we should just replace RegisterImageRequest with RegisterImageRequestIfAnimated. There shouldn't be any need for RegisterImageRequest at this point.

Comment 432

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #431)

> I'm not sure what this means. GetAnimated should fail until we know whether
> the image is animated, then it should succeed and return true if and only if
> the image is animated. So we should just be able to use it, we shouldn't
> have to do anything clever.

http://mxr.mozilla.org/mozilla-central/source/image/public/imgIContainer.idl#115

My understanding, from this comment, is that the only thing we can assume about GetAnimated is that if STATUS_DECODE_COMPLETE is set on the imgIContainer, then it will return a valid value. Otherwise, NS_ERROR_NOT_AVAILABLE will be returned. If NS_ERROR_NOT_AVAILABLE has been returned, my intention was that we would go ahead and register anyway, as once decoding is complete, OnStopDecode is going to deregister us. 

> Also I think we should just replace RegisterImageRequest with 
> RegisterImageRequestIfAnimated. There shouldn't be any need for RegisterImageRequest 
> at this point.

Sounds good, I'll make that change.

Comment 433

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Scott Johnson (:jwir3) from comment #432)
> My understanding, from this comment, is that the only thing we can assume
> about GetAnimated is that if STATUS_DECODE_COMPLETE is set on the
> imgIContainer, then it will return a valid value.

It says a little more than that, but basically you're right, if it might fail even when we have a second frame to display, then it's useless to us here.

So we need to fix that in imagelib. We need to change the contract for GetAnimated so that if the image needs an RequestRefresh, i.e. calling RequestRefresh might trigger invalidation due to another frame being available, GetAnimated will definitely succeed and return true.

> Otherwise,
> NS_ERROR_NOT_AVAILABLE will be returned. If NS_ERROR_NOT_AVAILABLE has been
> returned, my intention was that we would go ahead and register anyway, as
> once decoding is complete, OnStopDecode is going to deregister us.

But that brings us back to the performance issues we're worried about.

As for fixing GetAnimated:

VectorImage::GetAnimated is fine. It only fails if (mError || !mIsFullyLoaded). Nothing animates until the image is fully loaded (see VectorImage::ShouldAnimate), and if mError then mIsFullyLoaded can't be set. So it already meets the required contract.

RasterImage::GetAnimated is probably fine too, but I think it would be clearer to check ShouldAnimate() instead of checking mAnim (which is only created when ShouldAnimate() is true, but maybe GetAnimated could be called between ShouldAnimate() becoming true and mAnim being set up).

I think that's all that's needed, other than fixing the comment in imgIContainer.idl.

Comment 434

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #433)
> So we need to fix that in imagelib. We need to change the contract for
> GetAnimated so that if the image needs an RequestRefresh, i.e. calling
> RequestRefresh might trigger invalidation due to another frame being
> available, GetAnimated will definitely succeed and return true.

Hmm, that's not really enough though is it? We really need it to always succeed when the first OnStopFrame() happens (and afterward).

Again, that's fine for VectorImage. But it looks hard for RasterImage.

The simplest thing to do would probably be to add a new OnImageIsAnimated callback to imgIDecoderObserver, that's called once as soon as we discover the image is animated. Our listeners can add the image to the refresh driver in that callback.

Comment 435

[Scott Johnson (:jwir3)]

Attachment: Patch 1 (v12) - Interface changes for b666446 [r=dholbert,joe][sr=mats]

I'm adding the patches that changed the most to begin with - the interface, the implementation of Raster Image, and the layout utils class. This change to the interface adds a new method, OnImageIsAnimated, to imgIDecoderObserver, so that we can register when we are sure that the imahge is, in fact, animated.

Comment 436

[Scott Johnson (:jwir3)]

Attachment: Patch 9 (v13) - Implementation for RasterImage.

This is the implementation for RasterImage. It adds the implementation of notification for OnImageIsAnimated.

Comment 437

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v9) - Implementation for nsLayoutUtils. [r=roc]

The only changes I made were to remove the IfAnimated versions of the Register/Deregister functions. These are no longer needed, as I restructured the layout classes to no longer register during load.

Comment 438

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v7) - Implementation for nsImageLoadingContent. [r=roc][sr=mats]

Comment 439

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v9) - Implementation for nsImageBoxFrame. [r=roc]

Comment 440

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v11) - Implementation for nsBulletFrame. [r=roc]

Comment 441

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v10) - Implementation for nsImageLoader. [r=roc]

Comment 442

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v11) - Implementation for xultree. [r=roc]

Comment 443

[Joe Drew (not getting mail)]

Comment on attachment 572318 [details] [diff] [review]
Patch 1 (v12) - Interface changes for b666446 [r=dholbert,joe][sr=mats]

Review of attachment 572318 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgRequest.cpp
@@ +799,5 @@
> +NS_IMETHODIMP imgRequest::OnImageIsAnimated(imgIRequest *aRequest)
> +{
> +  NS_ABORT_IF_FALSE(mImage,
> +                    "OnImageIsAnimated callback before we've created our "
> +                    "image");

No need for this string literal to be on two lines.

::: image/src/imgStatusTracker.cpp
@@ +255,5 @@
>          proxy->OnStopFrame(frame);
>      }
> +
> +    // OnImageIsAnimated
> +    if (imageType == imgIContainer::TYPE_VECTOR

This isn't accurate. VectorImage calls mSVGDocumentWrapper->IsAnimated() for GetAnimated, so I presume that we should actually ask it whether it's animated.

I think we can actually ask the image via GetAnimated() always, and if it throws an error, just doin't send OnImageIsAnimated().

@@ +256,5 @@
>      }
> +
> +    // OnImageIsAnimated
> +    if (imageType == imgIContainer::TYPE_VECTOR
> +      || static_cast<RasterImage*> (mImage)->GetNumFrames() > 1) {

Incidentally, though I think this code will need to change due to the above comment, you should put || on the earlier line, and then line up the static_cast vertically below imageType. And you don't need a space between <RasterImage*> and (mImage).

::: widget/src/cocoa/nsMenuItemIconX.mm
@@ +560,5 @@
>    return NS_OK;
>  }
> +
> +NS_IMETHODIMP
> +nsMenuItemIconX::OnImageIsAnimated(imgIRequest*     aRequest)

weird indentation here.

Comment 444

[Joe Drew (not getting mail)]

Comment on attachment 572319 [details] [diff] [review]
Patch 9 (v13) - Implementation for RasterImage.

Review of attachment 572319 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsStubImageDecoderObserver.cpp
@@ +108,5 @@
>  
>  NS_IMETHODIMP
>  nsStubImageDecoderObserver::OnImageIsAnimated(imgIRequest *aRequest)
>  {
> +  printf("***** DEBUGjwir3: OnImageIsAnimated() called.\n");

plz to delete

::: image/src/Decoder.cpp
@@ +265,4 @@
>      mObserver->OnStopFrame(nsnull, mFrameCount - 1); // frame # is zero-indexed
> +    if (mFrameCount > 1 && !mIsAnimated) {
> +      mIsAnimated = true;
> +      mObserver->OnImageIsAnimated(nsnull);

Hm. Given that the common case is going to have nsnull passed as its imgIRequest parameter, why have the parameter at all?

::: image/src/RasterImage.h
@@ +85,5 @@
>  /**
> + * It would be nice if we had a macro for this in prtypes.h.
> + * TODO: Place this macro in prtypes.h as PR_UINT64_MAX.
> + */
> +#define UINT64_MAX_VAL PRUint64(-1)

Have you filed this bug on nspr?

Comment 445

[Scott Johnson (:jwir3)]

(In reply to Joe Drew (:JOEDREW!) (Unburying inbox) from comment #443)
> No need for this string literal to be on two lines.

Fixed.

> This isn't accurate. VectorImage calls mSVGDocumentWrapper->IsAnimated() for
> GetAnimated, so I presume that we should actually ask it whether it's
> animated.

As we discussed on IRC, I've fixed this in the latest version.
> Incidentally, though I think this code will need to change due to the above
> comment, you should put || on the earlier line, and then line up the
> static_cast vertically below imageType. And you don't need a space between
> <RasterImage*> and (mImage).

Yeah, this went away.

> weird indentation here.

Yeah, it's weird indentation, but I was mimicking the style of the file in which I found it. All of the other callback declarations have their arguments aligned. I can change it if you'd like, but it would be going against the consistency of the file... Maybe some kind of weird cocoa thing?

Comment 446

[Scott Johnson (:jwir3)]

(In reply to Joe Drew (:JOEDREW!) (Unburying inbox) from comment #444)
> plz to delete

Yep. It's gone. Sorry about that.

> Hm. Given that the common case is going to have nsnull passed as its
> imgIRequest parameter, why have the parameter at all?

Good question. I'll see if I can remove it the parameter. It might be that I had a reason for it once...

> Have you filed this bug on nspr?

Yes. Actually, it's landed already. Thanks for reminding me to take this extra macro out. :)

Comment 447

[Mats Palmgren (inactive)]

Comment on attachment 572318 [details] [diff] [review]
Patch 1 (v12) - Interface changes for b666446 [r=dholbert,joe][sr=mats]

> image/public/imgIContainer.idl
> image/public/imgIDecoderObserver.idl

bump the UUIDs

Comment 448

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #446)
> > Hm. Given that the common case is going to have nsnull passed as its
> > imgIRequest parameter, why have the parameter at all?
> 
> Good question. I'll see if I can remove it the parameter. It might be that I
> had a reason for it once...

So, it looks like I need it in nsImageLoadingContent to distinguish which (either mCurrentRequest or mPendingRequest) should be registered. However, I'm not sure how helpful this is, given that we're passing nsnull in as a parameter in Decoder. It looks like all of the callbacks in Decoder.cpp pass nsnull in as the imgIRequest parameter, though. See PostFrameStop() as an example: http://mxr.mozilla.org/mozilla-central/source/image/src/Decoder.cpp#264

I'm not sure why this is, but my guess is that the parameter is filled in at some point later during the event chain?

Comment 449

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We definitely need nsLayoutUtils::RegisterImageRequestIfAnimated which checks the animation status.

In the future, please don't reattach all the new patches at once, just one at a time with "is this OK?" will avoid cluttering up the bug.

Comment 450

[Scott Johnson (:jwir3)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #449)
> We definitely need nsLayoutUtils::RegisterImageRequestIfAnimated which
> checks the animation status.
> 

Ah, sorry... I misread this comment:
> Also I think we should just replace RegisterImageRequest with > > RegisterImageRequestIfAnimated. There shouldn't be any need for RegisterImageRequest
> at this point.

to mean that you'd rather the checks be done inline. 

> In the future, please don't reattach all the new patches at once, just one
> at a time with "is this OK?" will avoid cluttering up the bug.

Ok.

Comment 451

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v10) - Implementation for nsLayoutUtils. [r=roc]

Comment 452

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 572674 [details] [diff] [review]
Patch 3 (v10) - Implementation for nsLayoutUtils. [r=roc]

Review of attachment 572674 [details] [diff] [review]:
-----------------------------------------------------------------

This is good.

An unconditional RegisterImageRequest for use in OnImageIsAnimated is probably worth having too.

Comment 453

[Scott Johnson (:jwir3)]

Attachment: Patch 3 (v11) - Implementation for nsLayoutUtils. [r=roc]

> An unconditional RegisterImageRequest for use in OnImageIsAnimated is
> probably worth having too.

I agree. I've added back into this latest version.

Comment 454

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v8) - Implementation for nsImageLoadingContent. [r=roc][sr=mats]

This is the general idea I was thinking - unconditionally register in OnImageIsAnimated, and then conditionally register in the other situations where the image may need to be changed (in this case, FrameCreated). Let me know if you think this is ok.

Comment 455

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 572717 [details] [diff] [review]
Patch 4 (v8) - Implementation for nsImageLoadingContent. [r=roc][sr=mats]

Review of attachment 572717 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsImageLoadingContent.cpp
@@ +531,5 @@
> +  } else if (mPendingRequest) {
> +      nsLayoutUtils::RegisterImageRequestIfAnimated(presContext,
> +                                                    mPendingRequest,
> +                                                    &mPendingRequestRegistered);
> +  }

Should we actually register both?

@@ +548,5 @@
> +  } else if (mPendingRequest) {
> +    nsLayoutUtils::DeregisterImageRequest(GetFramePresContext(),
> +                                          mPendingRequest,
> +                                          &mPendingRequestRegistered);
> +  }

And deregister both here?

Comment 456

[Scott Johnson (:jwir3)]

Attachment: Patch 4 (v9) - Implementation for nsImageLoadingContent. [r=roc][sr=mats]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #455)
> Should we actually register both?
> And deregister both here?

Yes, I agree. I've changed the if statement from an else if to another if, so that both of them could be registered/deregistered, but only if they actually exist.

Comment 457

[Scott Johnson (:jwir3)]

Attachment: Patch 5 (v10) - Implementation for nsImageBoxFrame. [r=roc]

Comment 458

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 572949 [details] [diff] [review]
Patch 5 (v10) - Implementation for nsImageBoxFrame. [r=roc]

Review of attachment 572949 [details] [diff] [review]:
-----------------------------------------------------------------

You can send me the rest now :-)

Comment 459

[Scott Johnson (:jwir3)]

Attachment: Patch 6 (v12) - Implementation for nsBulletFrame. [r=roc]

OK, I'm sending the three remaining layout classes up for review now.

Comment 460

[Scott Johnson (:jwir3)]

Attachment: Patch 7 (v11) - Implementation for nsImageLoader. [r=roc]

Comment 461

[Scott Johnson (:jwir3)]

Attachment: Patch 8 (v12) - Implementation for xultree. [r=roc]

Comment 462

[Scott Johnson (:jwir3)]

Ok, I've pushed these changes to try again earlier this afternoon. Once these results come back verifying that we aren't introducing any new performance regressions, we can try again. (No pun intended).

Comment 463

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Great!

Comment 465

[Scott Johnson (:jwir3)]

Ok, I've pushed to try and received the results. It looks like the oranges still up here:

https://tbpl.mozilla.org/?tree=Try&rev=d7e3aa8f961d

are intermittent. 

In addition, I've prepared a spreadsheet of some of the most recent 7 runs of all talos tests over the past few days. I've compared each test, for each platform, with the results I received from the try talos tests. If the results of the test on try were within the limits of the minimum and maximum (well, actually, if it was less than the maximum, because if we're too fast, that's probably ok), then I marked it as 'good'. 

My results show that all tests are in the 'good' with one exception: dromeo_css on OS X Opt. The try push showed 3707.02 and the highest run time for that test that I found in the last few days was 3706.62, with an average run time of 3701.078. So, given that this is a 0.01% difference, we're probably ok. 

So, long story short, I'm going to try to land again this afternoon.

Comment 466

[Scott Johnson (:jwir3)]

Attachment: Talos Runs

This is the spreadsheet of talos run data I created to verify that we're not creating any new performance regressions. It was difficult to analyze what the graphs meant (jlebar and I are looking into this in a separate bug), so I found it easier just to create a chart of my own.

Comment 467

[Daniel Holbert [:dholbert]]

Landed updated patches, at jwir3's request:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8cf152adb60
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f8fbd2c0aa8
https://hg.mozilla.org/integration/mozilla-inbound/rev/cdbd6cf9f5f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/6de536c99c5d
https://hg.mozilla.org/integration/mozilla-inbound/rev/c301ad606217
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ebba3819ae6
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b8cba26cdb3
https://hg.mozilla.org/integration/mozilla-inbound/rev/6c7ff923785f
https://hg.mozilla.org/integration/mozilla-inbound/rev/eca4029b19d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5012d179aee

Comment 468

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/b8cf152adb60
https://hg.mozilla.org/mozilla-central/rev/3f8fbd2c0aa8
https://hg.mozilla.org/mozilla-central/rev/cdbd6cf9f5f4
https://hg.mozilla.org/mozilla-central/rev/6de536c99c5d
https://hg.mozilla.org/mozilla-central/rev/c301ad606217
https://hg.mozilla.org/mozilla-central/rev/3ebba3819ae6
https://hg.mozilla.org/mozilla-central/rev/9b8cba26cdb3
https://hg.mozilla.org/mozilla-central/rev/6c7ff923785f
https://hg.mozilla.org/mozilla-central/rev/eca4029b19d9
https://hg.mozilla.org/mozilla-central/rev/c5012d179aee

fwiw, to check Try Talos results you could have just used compare-talos against the parent changesets from the tree you pushed from... It usually works, I may blog post about that since looks like people miss this easy solution.

Comment 469

[Justin Lebar (not reading bugmail)]

<soapbox>Compare-talos is very broken right now.  See bug 696196.

Even without that bug, compare-talos does a poor job of understanding the baseline variance of tests, so it often flags as regressions things which are fine, and vice versa.  It catches big regressions, but to catch a small regression, I usually have to go into graphs-new.  But at that point, compare-talos is adding very little value to my experience.

We really need to have a better story for comparing Talos results.  See [1] for details.</soapbox>

[1] https://groups.google.com/d/msg/mozilla.dev.platform/kXUFafYInWs/CvHPZmxpiSwJ

Comment 470

[bzillamntr]

Been testing the latest nightly and this finally fixed GIFs (zero CPU wakeups for GIFs in background tabs, although GIFs scrolled out in the current tab still have a cost, unlike in WebKit).

Thanks a lot to everybody involved!

I see it missed the window for Firefox 10. I guess the answer is "no", but any chance it could make it to Aurora?

Comment 471

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to bzillamntr from comment #470)
> Been testing the latest nightly and this finally fixed GIFs (zero CPU
> wakeups for GIFs in background tabs,

Awesome

> although GIFs scrolled out in the
> current tab still have a cost, unlike in WebKit).

Yeah, this is covered by some combination of Bug 661304 and various other bugs.

> Thanks a lot to everybody involved!
> 
> I see it missed the window for Firefox 10. I guess the answer is "no", but
> any chance it could make it to Aurora?

Yeah, the answer is no, sorry.

But it'll only be delayed 6 weeks!

Comment 472

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 572319 [details] [diff] [review]
Patch 9 (v13) - Implementation for RasterImage.

This patch added PR_TRUE/PR_FALSE... Followup to fix, please.

Comment 473

[Scott Johnson (:jwir3)]

(In reply to Ms2ger from comment #472)
> This patch added PR_TRUE/PR_FALSE... Followup to fix, please.

Added new bug 702503 to track this. I'll post a patch momentarily.

Comment 474

[dindog]

after the recent patches landed, page with lots of GIF was getting better, but I notice a thing:

When Firefox run a while, the CPU usage of the same page with GIF bump up quite a lot. For example, the posting page contain some smiley face of 
http://forums.mozillazine.org/
Firefox take almost 0% CPU usage at the beginning, then I left the tab untouched, browsed something regular for a while, then switched back to the GIF tab, CPU usage was a lot higher(10%+), and it won't come down.

Comment 475

[Scott Johnson (:jwir3)]

(In reply to dindog from comment #474)
> after the recent patches landed, page with lots of GIF was getting better,
> but I notice a thing:
> 
> When Firefox run a while, the CPU usage of the same page with GIF bump up
> quite a lot. For example, the posting page contain some smiley face of 
> http://forums.mozillazine.org/
> Firefox take almost 0% CPU usage at the beginning, then I left the tab
> untouched, browsed something regular for a while, then switched back to the
> GIF tab, CPU usage was a lot higher(10%+), and it won't come down.

Hi -

I'm unable to reproduce on Nightly, Ubuntu Linux 11.04. What platform are you running on? Are others able to reproduce this?

Comment 476

[Scott Johnson (:jwir3)]

(In reply to dindog from comment #474)

Also, since this would be a regression as a result of this bug, it's probably best to file a new bug to track it, rather than posting in this bug's comments.

Comment 477

[dindog]

(In reply to Scott Johnson (:jwir3) from comment #476)
> (In reply to dindog from comment #474)
> 
> Also, since this would be a regression as a result of this bug, it's
> probably best to file a new bug to track it, rather than posting in this
> bug's comments.

win 7 sp1 32bit, see bug702892

Comment 478

[CruNcher]

Is it back again ? i see much higher cpu usage in November 17 again on the testcases compared with the fix also GPU usage is pretty high on d2d with GIF animated :(

Comment 479

[Scott Johnson (:jwir3)]

(In reply to CruNcher from comment #478)
> Is it back again ? i see much higher cpu usage in November 17 again on the
> testcases compared with the fix also GPU usage is pretty high on d2d with
> GIF animated :(

Hi CruNcher: 

This bug is probably pretty much finished. It would be advisable to file another bug if you're seeing issues, especially if you believe them to be regressions from this bug or another one. Feel free to CC me on it if you do end up filing another one.

Thanks!

Comment 480

[Alex Keybl [:akeybl]]

It sounds like this is still unresolved in Firefox 10. The patches here appear to be high risk for beta at this point (please correct me if I'm wrong). Do we still have options for resolving the issues in bug 691775 for FF10, or are we unconcerned with the perf hit on end users?

Comment 481

[Boris Zbarsky [:bzbarsky]]

Alex, bug 691775 was fixed for Firefox 10 by backing this bug out.  Then a fix was landed in this bug for Firefox 11 which does not cause the issues in bug 691775.

So I don't think there's anything here to worry about for either 10 or 11...

Comment 482

[Alex Keybl [:akeybl]]

(In reply to Boris Zbarsky (:bz) from comment #481)
> So I don't think there's anything here to worry about for either 10 or 11...

Thanks for helping to untangle the web of backouts :). Untracking for 10.